Guard provider submission success callbacks - #1363
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@codex review for provider success correctness, exactly-once conversion semantics, and missing regression coverage. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: df57935112
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| $status = is_object( $entry ) && isset( $entry->status ) ? $entry->status : ( is_array( $entry ) && isset( $entry['status'] ) ? $entry['status'] : null ); | ||
| if ( 'active' !== $status ) { | ||
| return; |
There was a problem hiding this comment.
On installations using Forminator versions whose Forminator_Form_Entry_Model predates the status property, successful entries reach this callback without that field and are now unconditionally discarded, disabling server-side submission actions and conversion tracking even though this integration declares no minimum Forminator version. Treat a missing status as the legacy entry shape—while checking legacy spam data where available—and reject only explicit non-success states; the new test currently codifies the regression by expecting a statusless object to be ignored.
AGENTS.md reference: AGENTS.md:L428-L432
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a23f680. Official WordPress.org Forminator SVN tags confirm the status property is absent through 1.44.3 and present by 1.48.3. Statusless legacy entries now pass only when is_spam is false and draft_id is empty; explicit non-active statuses remain rejected. Regression coverage now includes legacy success, legacy spam/draft, and current active/failure shapes.
|
@codex review for an exact-head follow-up on the legacy Forminator compatibility fix and the existing exactly-once success guards. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a23f680776
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| * @param array $field_data_array Field data array. | ||
| */ | ||
| public function on_success( $entry, $form_id, $field_data_array ) { | ||
| $status = is_object( $entry ) && isset( $entry->status ) ? $entry->status : ( is_array( $entry ) && isset( $entry['status'] ) ? $entry['status'] : null ); |
There was a problem hiding this comment.
When another plugin or a Forminator signature change invokes this hook with null or a scalar entry, both status checks fall through to the legacy path, where spam defaults to false and the draft ID to null, so the callback dispatches a submission and may run actions or record a conversion for a value that cannot represent a successful entry. Continue accepting statusless legacy objects, but return unless $entry is an object or supported array before applying the legacy fallback.
AGENTS.md reference: AGENTS.md:L428-L432
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 773a94f. The callback now rejects every non-object/non-array entry before applying legacy status fallback, with null and scalar regression cases. Focused PHP tests and PHPCS remain clean.
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
danieliser
changed the title
1 participant
Summary
Forminator and WS Form expose callbacks for intermediate persistence states as well as completed submissions. Popup Maker previously treated those callbacks uniformly, so Forminator drafts/abandoned entries and WS Form saves or failed validation could enter the normalized success flow and be counted as conversions.
This stacked correctness patch narrows the existing integrations to authoritative success states:
active.submitreceipts with a valid form ID and no native error or validation failures.wsf-submit-success; save receipts no longer produce normalized observations.The change intentionally adds no new public hook and does not alter action execution. It provides the correctness boundary required by the Pro provider capability adapters in PopupMaker/Pro#142.
Validation
vendor/bin/phpunit -c tests/php/phpunit.xml tests/php/tests/FormSubmissionPhases_Test.php— 13 tests, 41 assertionspnpm test:unit -- --runInBand tests/unit/wsforms-integration.test.js— 1 test passedgit diff --checkStack
Base:
feature/form-submission-phasesRelated Pro issue: PopupMaker/Pro#142