t531: fix set_demo_behavior fatal TypeError on PHP 8 when null passed via deprecated compat layer#914
Conversation
… via attributes()
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
inc/models/class-product.php (1)
1367-1376: Consider keeping: voidreturn type for consistency.The fix correctly resolves the PHP 8 TypeError. However, dropping the
: voidreturn declaration isn't necessary — an earlyreturn;is fully compatible withvoid. All sibling setters in this file (set_featured_image_id,set_slug,set_tax_category, etc.) retain: void, so preserving it here keeps the setter API consistent and still allows the nullable$behaviorparameter.♻️ Optional refactor
- public function set_demo_behavior($behavior) { + public function set_demo_behavior($behavior): void { if (null === $behavior) { return; }Note: the broader root cause (Base_Model::attributes() forwarding raw nulls into setters) means other strictly-typed setters in the codebase could be susceptible to the same fatal. Worth a follow-up audit of
set_*methods called throughattributes()/ the WU_Plan compat layer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@inc/models/class-product.php` around lines 1367 - 1376, Restore the explicit void return type on the setter to match sibling setters and keep the API consistent: change the method signature of set_demo_behavior to include ": void" while keeping the early "if (null === $behavior) { return; }" behavior and the existing assignments to $this->meta[self::META_DEMO_BEHAVIOR] and $this->demo_behavior; no other logic changes required.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@inc/models/class-product.php`:
- Around line 1367-1376: Restore the explicit void return type on the setter to
match sibling setters and keep the API consistent: change the method signature
of set_demo_behavior to include ": void" while keeping the early "if (null ===
$behavior) { return; }" behavior and the existing assignments to
$this->meta[self::META_DEMO_BEHAVIOR] and $this->demo_behavior; no other logic
changes required.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 07a25db1-6f25-4ba6-a596-0e644563fcf7
📒 Files selected for processing (1)
inc/models/class-product.php
🔨 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: |
|
Performance Test Results Performance test results for 3d4e5d6 are in 🛎️! Note: the numbers in parentheses show the difference to the previous (baseline) test run. Differences below 2% or 0.5 in absolute values are not shown. URL:
|
What
Remove the non-nullable
stringtype hint fromProduct::set_demo_behavior()and add a null guard, preventing aTypeErrorfatal on PHP 8.0+ whennullis passed through the genericBase_Model::attributes()setter dispatch.Why
Customers on PHP 8.x hit a fatal error loading any admin page that calls
wu_get_plan()through the deprecatedWU_Plancompat layer. The call chain:Three compounding causes:
set_demo_behavior(string $behavior)— non-nullable type hint on a public model setter, violating our coding standard (AGENTS.md: "NEVER add PHP return type declarations to public methods on model classes")null → ""coercion for typed parameters that PHP 7.4 allowed silentlyBase_Model::attributes()passes raw DB/compat-layer values to setters without null filteringHow
inc/models/class-product.php:1367stringparameter type and: voidreturn type (per coding standard — use@param/@returnPHPDoc instead)if (null === $behavior) { return; }guard — null from legacy data is silently ignored;get_demo_behavior()already falls back to'delete_after_time'when the property is unsetVerification
The fatal is reproducible by constructing a
Productviaattributes(['demo_behavior' => null]). After this fix, null is a no-op andget_demo_behavior()returns the default'delete_after_time'.Reported environment: PHP 8.5.5, WordPress 6.9.4, Ultimate Multisite 2.6.3.
aidevops.sh v3.8.94 plugin for OpenCode v1.3.17 with claude-sonnet-4-6 spent 4m and 6,734 tokens on this with the user in an interactive session.
Summary by CodeRabbit