fix(credits): make get_default_custom_credit_html public; revert defensive coerce from #1279#1295
Conversation
…nsive coerce from #1279 The Setup Wizard fatal that #1279 worked around had a single-character root cause, not a Field-layer defect: Credits::get_default_custom_credit_html was declared `protected`, but inc/class-credits.php:103 registers the field default as the array callable `[$this, 'get_default_custom_credit_html']`. `WP_Ultimo\Settings::save_settings()` resolves field defaults with `is_callable($field_default)` from a different class scope. PHP returns `false` for `is_callable([$instance, 'protected_method'])` evaluated outside the declaring class. With the callable unresolvable, the literal array `[$instance, 'get_default_custom_credit_html']` survived as the field value, landed in `Field::validate_textarea_field()`, hit `addslashes()`, and raised: Uncaught TypeError: addslashes(): Argument #1 ($string) must be of type string, array given in inc/ui/class-field.php:504 The same broken JSON also poisoned the wizard data-state JSON used to initialise the Vue form, which is why the form rendered with no settings fields on a fresh install. Both symptoms — empty form and the save fatal — disappear once the method is public. Changes: - inc/class-credits.php: visibility flipped from `protected` to `public`, with a docblock explaining the field-default callable contract so this bug class cannot silently regress. - inc/ui/class-field.php: reverts the defensive `coerce_textarea_value()` added by #1279. The function was a downstream band-aid; with the source fixed, no non-string value can reach this validator from this code path. Preserves the two unrelated PHPCS lint cleanups from #1279's first commit (alignment at line 301 and blank line before block comment at line 466). - tests/WP_Ultimo/Credits_Test.php: adds two regression tests: * `test_get_default_custom_credit_html_is_callable_externally` asserts both `is_callable([$instance, 'method'])` and ReflectionMethod::isPublic so a future visibility downgrade fails CI. * `test_get_default_custom_credit_html_returns_non_empty_string` asserts the resolved default value is a string suitable for textarea validation. - tests/WP_Ultimo/UI/Field_Test.php: reverts the six textarea-coerce tests added by #1279; they covered the removed band-aid. Audit: surveyed every `'default' => [$this, '...']` and `'options' => [$this, '...']` callable across inc/. `Credits::get_default_custom_credit_html` was the only target with restricted visibility. The two other registered field defaults (`Domain_Manager::default_domain_mapping_instructions`, `Settings::get_default_company_country`) and all seven options callables are already public — no further code changes needed. Verification: vendor/bin/phpunit --filter 'Credits_Test' # 17/17 pass vendor/bin/phpcs inc/class-credits.php inc/ui/class-field.php \ tests/WP_Ultimo/Credits_Test.php # 0 errors vendor/bin/phpstan analyse inc/class-credits.php \ inc/ui/class-field.php tests/WP_Ultimo/Credits_Test.php # No errors Live-install repro on a fresh install before/after the change: - Before: wizard step 2 renders with no fields; submitting fatals at class-field.php:504 with the addslashes TypeError above. - After: all 11 fields render and save without error.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughThe PR exposes ChangesCallable Default Method and Textarea Validation Simplification
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
🔨 Build Complete - Ready for Testing!📦 Download Build Artifact (Recommended)Download the zip build, upload to WordPress and test:
🌐 Test in WordPress Playground (Very Experimental)Click the link below to instantly test this PR in your browser - no installation needed! Login credentials: |
Summary
Root cause
'default' => [$this, 'get_default_custom_credit_html'],But // inc/class-settings.php (save_settings):
if (is_callable($field_default)) {
$field_default = call_user_func($field_default);
}PHP returns The same unresolved-callable array also poisoned the Audit of related patternsSurveyed every
Changes
Verificationvendor/bin/phpunit --filter Credits_Test
# 17 tests, 28 assertions, OK
vendor/bin/phpcs inc/class-credits.php inc/ui/class-field.php \
tests/WP_Ultimo/Credits_Test.php tests/WP_Ultimo/UI/Field_Test.php
# 0 errors
vendor/bin/phpstan analyse inc/class-credits.php inc/ui/class-field.php \
tests/WP_Ultimo/Credits_Test.php
# OK No errorsLive-install verification on a fresh install:
Why revert #1279 instead of keeping it as defence-in-depthThe coerce in
|
superdav42
added
the
review-feedback-scanned
1 participant
Summary
PR #1279added a defensive coerce inField::validate_textarea_field()to swallow theSetup Wizard fatal that affected fresh installs. That band-aid masked a one-character
root cause in
inc/class-credits.php. This PR reverts the band-aid and fixes the source.Root cause
inc/class-credits.php:103registers thecredits_custom_htmlfield's default as anarray callable:
But
get_default_custom_credit_html()was declaredprotected. The consumer thatneeds to invoke it lives in a different class:
PHP returns
falseforis_callable([$instance, 'protected_method'])evaluated outsidethe declaring class scope. So
Settings::save_settings()saw the callable as notcallable, never invoked it, and let the literal array
[$instance, 'method-name']flow through as the field value into
Field::validate_textarea_field()→addslashes(array)→ fatal:The same unresolved-callable array also poisoned the
data-stateJSON the SetupWizard's Vue form is initialised from, which is why on a fresh install the wizard
step rendered with no settings fields at all. Clicking Continue still POSTed
and triggered the fatal.
Both symptoms — empty form and the save fatal — disappear with
protected→public.Audit of related patterns
Surveyed every
'default' => [$this, '...']and'options' => [$this, '...']callable registration across
inc/.Credits::get_default_custom_credit_htmlwasthe only target with restricted visibility. All other registered field-default
and field-options callables are already
public:inc/class-credits.php:103get_default_custom_credit_htmlprotected→ nowpublicinc/managers/class-domain-manager.php:555default_domain_mapping_instructionspublic✓inc/class-settings.php:715get_default_company_countrypublic✓inc/ui/class-template-switching-element.php:193get_template_selection_templatespublic✓inc/checkout/signup-fields/class-signup-field-order-summary.php:182get_templatespublic✓inc/checkout/signup-fields/class-signup-field-period-selection.php:174get_template_optionspublic✓inc/checkout/signup-fields/class-signup-field-steps.php:168get_templatespublic✓inc/checkout/signup-fields/class-signup-field-template-selection.php:253get_template_selection_templatespublic✓inc/checkout/signup-fields/class-signup-field-pricing-table.php:231get_pricing_table_templatespublic✓inc/managers/class-gateway-manager.php:388get_gateways_as_optionspublic✓No other code changes needed.
Changes
inc/class-credits.php— flipget_default_custom_credit_htmlfromprotectedto
public. Adds a docblock explaining the field-default callable contract so afuture visibility downgrade is caught at review time (and by the new regression
test below).
inc/ui/class-field.php— revert thecoerce_textarea_value()defensive layerfrom Fix/textarea array coercion #1279. Preserves the two unrelated PHPCS lint cleanups (alignment + blank-line)
from Fix/textarea array coercion #1279's first commit.
tests/WP_Ultimo/Credits_Test.php— adds two regression tests:test_get_default_custom_credit_html_is_callable_externallyasserts bothis_callable([$instance, 'method'])andReflectionMethod::isPublicso afuture visibility downgrade fails CI.
test_get_default_custom_credit_html_returns_non_empty_stringconfirms theresolved default value is a string.
tests/WP_Ultimo/UI/Field_Test.php— reverts the six textarea-coerce testsadded by Fix/textarea array coercion #1279; they exercised the removed band-aid.
Verification
Live-install verification on a fresh install:
step=your-companyform renderingTypeError: addslashes(): … array giveninclass-field.php:504wp-content/debug.logWhy revert #1279 instead of keeping it as defence-in-depth
The coerce in
validate_textarea_field()silently dropped any non-string valueto
''. With the source bug fixed, no non-string value reaches this validatorfrom this code path. Keeping a silent coercer here would hide a future
recurrence of the same bug class instead of failing loudly. The visibility
contract + regression test is the better signal.
Summary by CodeRabbit
Refactor
Tests