rework ACF fields processing (WP-991)

[vsolovei-smartling]

No description provided.

[sl-mmuradov]

Should it return? Can it just clamp the value?

[vsolovei-smartling]

Not sure I've understand the clamping part, the return causes the function to stop any code below from running, so any deeper nested elements are not processed

[sl-mmuradov]

Looks weird. Hope it is done this way intentionally )

[vsolovei-smartling]

Collecting definitions is an expensive action, so we don't load them every time plugin loads. External callers should not explicitly call run or runIfRequired. Reworked

[sl-mmuradov]

Can we call runIfRequired here to reduce similar code?

[vsolovei-smartling]

Done

[sl-mmuradov]

How did it work before? Was $fieldKey boolean? It is still boolean?

[vsolovei-smartling]

Simplified code

[sl-mmuradov]

What is purpose to check return value for null here?

[vsolovei-smartling]

CustomFieldFilterHandler::getProcessor return false when a processor is not found, and null is used in newer code. This function must return a null or MetaFieldProcessorInterface, returning false would result in a fatal error.

[PavelLoparev]

Issues

🔴 Critical

inc/Smartling/WP/Table/QueueManagerTableWidget.php:276 — Float 0.001 passed to acquireLock() which has int $ttlSeconds type hint.

Without declare(strict_types=1), PHP silently coerces 0.001 → 0, making the lock expire immediately — the duplicate-run guard never fires. The test covering this call path (testPrepareItemsShowsRunningMessageWhenSdkWrapsGuzzle423) was deleted in this same PR, so there is no safety net.

// Fix:
$this->api->acquireLock($profile, JobAbstract::CRON_FLAG_PREFIX . $jobName, 1);

---

🟡 Warning

inc/Smartling/Extensions/Acf/AcfDynamicSupport.php:141,174 — acf_get_fields() returns false when a group has no fields; iterating false with foreach raises a PHP warning.

// Fix:
foreach ((acf_get_fields($group) ?: []) as $field) {

inc/Smartling/Extensions/Acf/AcfDynamicSupport.php — loadDefinitions — If ACF is inactive, $this->definitions stays null after tryRegisterACF, so runIfRequired() re-enters run() on every subsequent call. Fix: assign $this->definitions = [] in the else branch.

tests/Smartling/Extensions/Acf/AcfTypeDetectorTest.php:testGetProcessorForGutenberg — Test still sets the $acf_stores global which is now dead code after the refactor to acf_get_field_groups(). The test is fragile and its intent is unclear.

inc/Smartling/Helpers/RelativeLinkedAttachmentCoreHelper.php:63-65 — The count($this->acfDefinitions) === 0 guard is now indistinguishable between "not loaded" and "ACF has no fields," causing redundant run() calls. Use runIfRequired() instead.

inc/Smartling/Extensions/Acf/AcfDynamicSupport.php:addAcfFieldToDefs — Container fields with both a DB ID and inline sub_fields will have sub-fields processed twice (once from DB, once from inline array).

---

🔵 Suggestion

inc/Smartling/ContentTypes/Elementor/ElementAbstract.php:113 — assert() is disabled in production (zend.assertions = -1). Replace with an explicit instanceof check and log/exception.

inc/Smartling/DbAl/WordpressContentEntities/PostEntityStd.php:106 — get_edit_post_link() default context changed from 'edit' to 'display' — different URL encoding. Verify intent.

inc/Smartling/Extensions/Acf/AcfDynamicSupport.php — getDefinitions() returns [] for both "not loaded" and "zero fields" — misleading API. Consider returning null when uninitialized.

---

Recommendations

Behavioral change — local ACF field groups: The old code handled PHP/JSON-registered ACF groups via $acf_local/$acf_stores. The new code skips groups where groupId <= 0, meaning fields defined via register_field_group() without a DB post will no longer appear in $definitions. Needs product/QA sign-off.

Test coverage gap: testPrepareItemsShowsRunningMessageWhenSdkWrapsGuzzle423 was deleted with no replacement. The behavioral change that motivated it makes sense, but the regression path is now uncovered.

Strict types: None of these files declare strict_types=1 — the critical float-to-int coercion bug is a direct consequence.

---

Assessment

Ready to merge? No — With fixes

Reasoning: The float 0.001 silently coercing to int 0 disables the distributed lock probe in QueueManagerTableWidget, which is the production guard against concurrent job runs — a silent behavioral regression that could cause duplicate job execution. The acf_get_fields() returning false without a null guard will also produce PHP warnings in any field group with zero children.

---

🤖 Review generated with Claude Code (claude-pr-review-local skill)

[vsolovei-smartling]

Issues

🔴 Critical

inc/Smartling/WP/Table/QueueManagerTableWidget.php:276 — Float 0.001 passed to acquireLock() which has int $ttlSeconds type hint.
Merged master into this branch