Prevent issues when license validation request fails

[Crabcyborg]

Related issue https://github.com/Strategy11/formidable-pro/issues/6610

[coderabbitai]

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

• Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[deepsource-io]

DeepSource Code Review

We reviewed changes in 2a2d8c1...0c6984d on this pull request. Below is the summary for the review, and you can see the individual issues we found as inline review comments.

See full review on DeepSource ↗

PR Report Card

Overall Grade

Security   Reliability   Complexity   Hygiene

Code Review Summary

Analyzer	Status	Updated (UTC)	Details
PHP		Sep 16, 2026 7:48p.m.	Review ↗
JavaScript		Sep 16, 2026 7:48p.m.	Review ↗

---

Important

AI Review is run only on demand for your team. We're only showing results of static analysis review right now. To trigger AI Review, comment @deepsourcebot review on this thread.

[deepsource-io]

Call to an undefined method test_FrmAddon::getMockBuilder()

The method you are trying to call is not defined, which can result in a fatal error.

[deepsource-io]

Call to an undefined method test_FrmAddon::never()

The method you are trying to call is not defined, which can result in a fatal error.

[deepsource-io]

Call to an undefined method test_FrmAddon::once()

The method you are trying to call is not defined, which can result in a fatal error.

[deepsource-io]

Call to an undefined method test_FrmAddon::getMockBuilder()

The method you are trying to call is not defined, which can result in a fatal error.

[deepsource-io]

Call to an undefined method test_FrmAddon::never()

The method you are trying to call is not defined, which can result in a fatal error.

[deepsource-io]

Call to an undefined method test_FrmAddon::once()

The method you are trying to call is not defined, which can result in a fatal error.

[deepsource-io]

Call to an undefined method test_FrmAddon::getMockBuilder()

The method you are trying to call is not defined, which can result in a fatal error.

[deepsource-io]

Call to an undefined method test_FrmAddon::never()

The method you are trying to call is not defined, which can result in a fatal error.

[deepsource-io]

Call to an undefined method test_FrmAddon::once()

The method you are trying to call is not defined, which can result in a fatal error.

[codecov]

Codecov Report

❌ Patch coverage is 38.88889% with 22 lines in your changes missing coverage. Please review.
✅ Project coverage is 28.61%. Comparing base (943db5f) to head (0c6984d).
⚠️ Report is 212 commits behind head on master.

Files with missing lines	Patch %	Lines
classes/models/FrmAddon.php	38.88%	22 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #3346      +/-   ##
============================================
+ Coverage     28.33%   28.61%   +0.27%     
- Complexity     9677     9854     +177     
============================================
  Files           159      160       +1     
  Lines         32597    33051     +454     
============================================
+ Hits           9236     9456     +220     
- Misses        23361    23595     +234     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[franky-the-going-merry]

Request Changes.

The inconclusive idea is right, and the path it fixes is real — a JSON body like {"code":500} used to land on the 'missing' placeholder and make is_license_revoked() clear a working license. But the way "did the API answer?" is decided introduces the opposite failure, and the two changes that actually stop the license being wiped have no test.

I checked the status vocabulary against the license server's own code (edd-software-licensing v3.9.7, src/Licenses/Actions/Activate.php::get_activation_result()) rather than inferring it from this file.

Blocking

1. get_reported_license_statuses() omits four rejection codes the server really returns — missing_item_id, key_mismatch, missing_url, bundle_activation_not_allowed. A definitive "no" is now recorded as "couldn't reach the API," and the rejected key is kept. FrmAddon.php:1228
2. 'missing' is both the inconclusive placeholder and a real verdict, so an inconclusive check shows the user "That license key is invalid." The PR's own test asserts this combination. FrmAddon.php:1254
3. activate_license() persists the new key before the check runs, so the guard leaves ..._license_key and ..._license_active describing two different licenses, with no path back. FrmAddon.php:1189
4. Neither maybe_set_active()'s new guard nor activate_defined_license()'s new condition is covered. test_FrmAddon.php:190

Non-blocking

• The 429 guard in is_license_revoked() is now unreachable. FrmAddon.php:1022
• The providers use a payload shape send_mothership_request() doesn't produce for these statuses, so the path that actually fires revocation clearing in production is untested. test_FrmAddon.php:200
• @since x.x appears 9 times, all of them new here; nothing else in the repo uses it, and the plugin is at 6.35.
• formidable-pro#6610's request 1 is the silent Dynamic-to-text field-type fallback itself. This fixes one upstream cause of the unauthorized state, not that fallback — it still happens on a genuinely expired license. Worth not auto-closing the issue on merge.

CI is green across the board (PHPUnit on PHP 7.4 and 8, PHPCS, PHP CS Fixer, Rector, ESLint, Oxlint, Stylelint). DeepSource's three "undefined method" hits on getMockBuilder/once/never are false positives — the same calls already exist in this file's setUp() before the PR. No new security surface in the diff: no user input, query, or output rendering changed; the security-shaped risk here is licensing integrity, which is finding 1.

Still marked draft — flagging in case the label went on earlier than intended.

[franky-the-going-merry]

Blocking. This allowlist is missing four rejection codes the license server actually returns, and every omission fails open.

Ground truth is edd-software-licensing v3.9.7, src/Licenses/Actions/Activate.php::get_activation_result(). Its complete error set for edd_action=activate_license is:

missing, missing_item_id, disabled, key_mismatch, expired,
missing_url, bundle_activation_not_allowed, no_activations_left,
invalid_item_id, item_name_mismatch

missing_item_id, key_mismatch, missing_url and bundle_activation_not_allowed are not in this list. send_mothership_request() collapses an error response to that bare string at :1435:

$message = is_array( $json_res ) && isset( $json_res['error'] ) ? $json_res['error'] : $json_res;

so each of those four now takes the elseif ( $status ) branch at :1292 with inconclusive left true.

Concrete failure: a customer activates a bundle license. The server replies bundle_activation_not_allowed. maybe_set_active( false, false ) returns early, so the rejected key stays in edd_<slug>_license_key, the previous active flag is untouched, and the user sees the raw code string because get_messages() has no entry for it. Before this PR that same response cleared the license. key_mismatch is the everyday version of the same thing — a key pasted with the wrong case or stray whitespace.

The list has the opposite problem too: revoked, blocked, inactive and site_inactive aren't in activate_license's result set at all.

Root fix: don't classify on the status string. The transport already knows, and send_mothership_request() already computes it — is_wp_error( $resp ), and wp_remote_retrieve_response_code( $resp ), which it stores as $this->save_status['response_code']. That is the same value is_license_revoked() reads at :1022. A reply that parsed as JSON from the server is conclusive whatever string it carries; a WP_Error or an unparseable body is not. That derivation can't drift when the server adds a code — this list will, and silently.

If the list stays for now, it at least needs the four missing codes:

Suggested change

	return array(
	'valid',
	'invalid',
	'expired',
	'revoked',
	'blocked',
	'disabled',
	'missing',
	'inactive',
	'site_inactive',
	'no_activations_left',
	'invalid_item_id',
	'item_name_mismatch',
	);
	return array(
	'valid',
	'invalid',
	'expired',
	'revoked',
	'blocked',
	'disabled',
	'missing',
	'missing_url',
	'missing_item_id',
	'key_mismatch',
	'bundle_activation_not_allowed',
	'inactive',
	'site_inactive',
	'no_activations_left',
	'invalid_item_id',
	'item_name_mismatch',
	);

[franky-the-going-merry]

Blocking. 'missing' can't be both the "no answer yet" placeholder and a real verdict.

It's a genuine server status — get_activation_result() returns error => 'missing' for a key that doesn't exist — it has an entry in get_messages(), and is_license_revoked() at :1027 treats it as grounds to clear. It is also the value $response['status'] keeps when nothing overwrites it.

The PR's own test pins the collision. license_status_provider() asserts array( 'code' => 500 ) produces inconclusive = true, status = 'missing'. Follow that through activate_license(): $response['error'] is false, so it takes the else branch at :1175, $messages['missing'] exists, and the user is told "That license key is invalid" for a check that never reached a verdict. That's the same wrong-conclusion-from-a-failed-request bug this PR exists to fix, just moved from the stored state to the message.

Use a placeholder that can't be mistaken for a verdict:

Suggested change

	$response = array(
	'status' => 'missing',
	'error' => true,
	'status' => 'missing',
	'error' => true,
	'inconclusive' => false,
	);
	$response = array(
	'status' => '',
	'error' => true,
	'inconclusive' => false,
	);

That needs the if ( ! $this->license ) branch at :1259 to set 'missing' explicitly before returning (that one is a real verdict), and activate_license() to pick a connection-problem message when inconclusive is set.

Separately, 'error' here is dead. Every return path sets it to false — :1260 on the no-license branch, and :1267 as the first statement of the try, before anything can throw — so activate_license()'s if ( $response['error'] ) at :1173 never runs. That flag was already meant to carry "the request didn't work"; the PR adds inconclusive beside it instead of repairing it. Worth collapsing to one flag while this area is open.

[franky-the-going-merry]

Blocking. This guard protects the active option, but the key was already overwritten before the request went out, so an unreachable API leaves the two describing different licenses.

activate_license() opens with:

$this->set_license( $license );   // :1162 — writes edd_<slug>_license_key
$this->license = $license;

When the check comes back inconclusive, maybe_set_active( false, false ) returns without touching edd_<slug>_license_active.

So: a site licensed under key A, someone pastes key B, the API happens to be unreachable. Result — key = B (never validated), active = valid (earned by A). The site reports itself licensed under a key the server never accepted.

There is no self-correction path. is_license_revoked() throttles on transient_key(), an md5 of the license, so B starts with a clean throttle — but when that weekly check does run and B comes back invalid, :1027 only clears on revoked/blocked/disabled/missing. invalid isn't in that set, so the stale active flag stays indefinitely.

Fix: don't persist the key until the check is conclusive. FrmAddon::set_license() is called from exactly one place — :1162 — so moving it below the get_license_status() call has no other caller to account for. Failing that, capture the previous key at the top of activate_license() and restore it when maybe_set_active() bails.

[franky-the-going-merry]

Non-blocking: this 429 guard is now unreachable.

A 429 from the license server can't produce an allowlisted status. send_mothership_request() returns either There was a 429 error: ... for a non-JSON body, or the JSON's error value — and no rate-limit code is in get_reported_license_statuses(). Either way inconclusive stays true and the new check at :1016 returns first.

That's the correct outcome, but leaving the dead branch below implies a case that can no longer reach it. Drop it — or, if the intent is to keep response-code handling explicit, that's an argument for deriving inconclusive from $this->save_status['response_code'] in the first place (see the comment on get_reported_license_statuses()).

[franky-the-going-merry]

Blocking: the two changes that actually fix the reported bug have no test.

The new coverage is for get_license_status() and is_license_revoked(). Neither is where the license was getting wiped. That happens in maybe_set_active() — the new if ( ! $is_valid && ! $is_conclusive ) { return; } guard, reached through activate_license() — and in activate_defined_license()'s new && empty( $response['inconclusive'] ). Codecov agrees: 38.89% patch coverage, 22 lines missing.

get_licensed_addon() already provides everything needed. Two cases:

• activate_license() with a connection-error payload, on an addon whose edd_<slug>_license_active is already valid — assert that option and edd_<slug>_license_key both survive the call.
• activate_defined_license() with the same payload — assert it returns the defined key rather than ''.

Run the first against master before adding it and confirm it fails there. That's what makes it a regression test rather than a restatement of the new code.

[franky-the-going-merry]

Non-blocking, but it narrows what this suite proves: these payloads aren't the shape production produces for these statuses.

send_mothership_request() only returns an array when the decoded body has no error key (:1435):

$message = is_array( $json_res ) && isset( $json_res['error'] ) ? $json_res['error'] : $json_res;

Every rejection the server reports arrives through that ternary's true branch, as a bare string. So array( 'license' => 'revoked' ) isn't what a revoked license looks like here — 'revoked' is.

That matters specifically for these four rows. Before this PR the array branch forced anything other than valid/invalid to 'missing', so the string path was the only way is_license_revoked()'s clear ever fired in production. All four rows that assert clearing use the array shape; the string shape appears only in rows that assert not clearing. Add string rows for at least revoked, disabled and missing in both providers.

Two smaller things in the same block:

• yield 'no body' => array( null, false ) at :211 models a return send_mothership_request() can't produce — it always returns at least the 'Your License Key was invalid' default. The row passes without proving anything.
• Both providers are annotated @return void mixed>, mixed>> (:196, and the matching line in license_status_provider()) while actually returning \Iterator. Looks like a truncated generic that should read @return \Iterator<string, array<int, mixed>>. get_licensed_addon()'s @return PHPUnit\Framework\MockObject\MockObject also needs a leading backslash.

[vivi-the-going-merry]

Out of scope for a Vivi pickup: this is Crabcyborg's own draft PR, still actively being iterated on. Franky's CHANGES_REQUESTED already routed it back to him as an ordinary review (removed franky-review, no vivi-pickup handoff filed) — the PR-handoff protocol only fires for an orphaned author or a Vivi-authored PR, neither of which applies here. vivi-pickup was added by robin-the-going-merry[bot], not Franky, with no accompanying comment. Clearing the label rather than pushing onto an active author's in-progress branch.