Try to cut back on calls to get option

[Crabcyborg]

Related ticket https://secure.helpscout.net/conversation/3168224965/244001

• Ensures that the license check is part of the "all options" data to avoid extra db calls.
• Avoids get_option checks for the request lock when the cached data is not expired.

Pre-release
formidable-6.26.1b.zip

[coderabbitai]

Warning

Rate limit exceeded

@Crabcyborg has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 7 minutes and 4 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between c44be72 and b449ec9.

📒 Files selected for processing (1)

• classes/models/FrmFormApi.php (2 hunks)

Walkthrough

Single-file changes in classes/models/FrmFormApi.php adjusting internal cache handling: stricter expired-cache checks during in-flight requests, a small local variable addition in option retrieval, and altered autoload flag determination when writing non-multisite options.

Changes

Cohort / File(s)

Summary

Caching logic adjustments classes/models/FrmFormApi.php

- get_cached: adds explicit is_expired check and branches: if expired and not running → return false; if expired and running → continue but require cached data to match current plugin version (return false if not). Adjusts expired-cache handling during in-flight requests. - get_cached_option: introduces a local $key assignment before multisite handling (no behavioral effect). - set_cached: on non-multisite installs, switches from update_option(..., 'no') to using an autoload flag derived from cache key prefix (autoload when key starts with frm_addons_l). Adds comment about autoloading license cache.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

• Review expired-cache branching in get_cached (behavior when requests are running vs not).
• Verify version check when using expired cache during in-flight requests.
• Inspect set_cached autoload flag logic and ensure it matches intended prefixes and multisite behavior.
• Note the unused local $key in get_cached_option for potential cleanup or future use.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	No pull request description was provided by the author, making it impossible to assess whether any description exists or is related to the changeset.	Add a pull request description that explains the motivation, approach, and impact of the changes to cache handling and option calls.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'Try to cut back on calls to get option' accurately reflects the main change: optimizing cache handling and autoload behavior to reduce get_option calls.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

---

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 and usage tips.}

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

classes/models/FrmFormApi.php (1)

408-415: Clear memoized cache on reset.

The reset_cached() method deletes the persistent cache but doesn't clear $memoized_api_info. Subsequent calls to get_cached_option() will return stale memoized data instead of recognizing the cache was reset.

Apply this diff to clear the memoized cache:

 	public function reset_cached() {
+		unset( self::$memoized_api_info[ $this->cache_key ] );
+
 		if ( is_multisite() ) {
 			delete_site_option( $this->cache_key );
 		} else {
 			delete_option( $this->cache_key );
 		}
 		$this->done_running();
 	}

🧹 Nitpick comments (1)

classes/models/FrmFormApi.php (1)

314-328: Smart optimization for concurrent request handling.

The modified logic correctly allows using expired cache when an API request is in-flight, preventing redundant requests while maintaining version consistency. This is a solid improvement.

Minor nitpick: The comment on line 320 duplicates the one on line 313.

Consider removing the duplicate comment:

-		// If the api call is running, we can use the expired cache.
 		$version     = FrmAppHelper::plugin_version();

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 32b847c and 8df5486.

📒 Files selected for processing (1)

• classes/models/FrmFormApi.php (4 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

classes/models/FrmFormApi.php (5)

classes/models/FrmInbox.php (1)

• is_expired (243-245)

classes/controllers/FrmFormTemplatesController.php (1)

• is_expired (861-863)

classes/models/FrmAddon.php (1)

• is_running (1085-1087)

classes/models/FrmForm.php (1)

• get_option (1336-1341)

classes/models/FrmField.php (1)

• get_option (1486-1494)

🪛 GitHub Actions: Inspections

classes/models/FrmFormApi.php

[error] 1-1: PHP CS Fixer reported issue: blank_line_before_statement (1/492 files) in classes/models/FrmFormApi.php. Run './vendor/bin/php-cs-fixer fix' to apply fixes. Process exited with code 8.

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)

• GitHub Check: Cypress
• GitHub Check: PHP 8 tests in WP trunk
• GitHub Check: PHP 7.4 tests in WP trunk
• GitHub Check: PHP 7.4 tests in WP trunk
• GitHub Check: PHP 8 tests in WP trunk
• GitHub Check: Cypress

🔇 Additional comments (3)

classes/models/FrmFormApi.php (3)

30-37: Well-structured memoization property.

The static property is appropriately scoped and documented. The array initialization and indexing by cache key will effectively reduce redundant get_option calls.

---

343-360: Effective memoization implementation.

The early return pattern efficiently avoids redundant option lookups, and the multisite fallback logic is correctly preserved.

---

376-377: Memoization update looks good.

Correctly updates the in-memory cache when setting the persistent cache.

[coderabbitai]

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8df5486 and c44be72.

📒 Files selected for processing (1)

• classes/models/FrmFormApi.php (3 hunks)

🧰 Additional context used

🪛 GitHub Actions: Inspections

classes/models/FrmFormApi.php

[error] 1-1: PHP-CS-Fixer: 1 issue found (blank_line_before_statement). Command: './vendor/bin/php-cs-fixer fix --dry-run --verbose'.

🪛 PHPMD (2.15.0)

classes/models/FrmFormApi.php

334-334: Avoid unused local variables such as '$key'. (undefined)

(UnusedLocalVariable)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)

• GitHub Check: PHP 8 tests in WP trunk
• GitHub Check: PHP 7.4 tests in WP trunk
• GitHub Check: Cypress

🔇 Additional comments (2)

classes/models/FrmFormApi.php (2)

305-319: LGTM! Cleaner expired-cache handling.

The refactored logic improves readability by extracting the expiration check into $is_expired and properly short-circuits when the cache is still valid, avoiding unnecessary is_running() calls.

---

364-366: Autoload logic is correct but always evaluates to true for this class.

Given that set_cache_key() always sets $this->cache_key to start with 'frm_addons_l', the strpos check will always return 0, making $autoload always true. This is fine if you're being defensive for potential subclasses that override set_cache_key(). Otherwise, consider simplifying to just true.