Bugfix/base gateway return type compat#781
Conversation
External gateway plugins that extend Base_Gateway without matching return type declarations cause a PHP fatal error. Removing return types from the abstract base class restores compatibility with third-party gateways while keeping the concrete implementations (Stripe, PayPal) typed.
…store addon compatibility PHP enforces that child class method signatures must be compatible with parent declarations. When a parent adds a return type (e.g. ': string'), any existing child class that overrides that method without the return type causes a fatal Compile Error on PHP 8.x. This broke the WooCommerce addon v2.0.22 (and potentially other addons) when upgrading to UM v2.5.1, because Base_Gateway added ': string' to get_payment_url_on_gateway() and other methods that addons override. Removes return type declarations from all overridable public methods in: - Base_Gateway (12 methods) + all bundled gateway subclasses - Base_Element (17 methods) + all bundled element subclasses - Base_Model (6 methods) - Base_Host_Provider (11 methods) + DNS_Provider_Interface - Base_Capability_Module (7 methods) + Capability_Module interface - Base_Signup_Field (1 method) Root cause: two commits added return types to existing public APIs: - b41dc2b 'Use PHP 7.4 features' added void returns to setters - b8df0b8 'Add PayPal REST gateway' added string returns to URL methods PHPDoc @return tags are preserved for IDE/static analysis support. Bundled subclasses that override these methods also have their return types removed for consistency. External addon subclasses that already added matching return types will continue to work (child can be more specific than parent). Reported-by: Kenedy (WC addon v2.0.22 fatal on UM v2.5.1)
…ype regressions Adds a class-level @extensible PHPDoc tag to all 6 base/abstract classes that external addons extend. The annotation warns developers and AI agents not to add PHP return type declarations to public methods on these classes. Also updates AGENTS.md to replace the old 'Return type void on init methods' instruction (which caused this bug) with an explicit prohibition on adding return types to extensible base classes. Defense layers: 1. AGENTS.md instruction (catches AI agents at prompt level) 2. @extensible PHPDoc on each class (catches humans reading the code) 3. @return PHPDoc tags preserved (gives IDEs/PHPStan type info without the PHP-enforced contract that breaks child classes)
📝 WalkthroughWalkthroughThis PR systematically removes PHP return type declarations and some parameter type hints from public methods in base/abstract classes and interfaces across multiple directories, implementing updated coding standards to maintain compatibility with external addon implementations that override these methods. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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: |
|
Performance Test Results Performance test results for 29e8710 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:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
inc/ui/class-billing-info-element.php (1)
267-272:⚠️ Potential issue | 🟡 MinorFix PHPDoc return contract for
output().Line 269 declares
@return string, but this method renders via side effects and uses barereturn;. The PHPDoc should reflectvoid.Proposed patch
- * `@return` string + * `@return` void🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@inc/ui/class-billing-info-element.php` around lines 267 - 272, Update the PHPDoc for the method output() in class Billing_Info_Element (the output method) to reflect that it does not return a string but renders via side effects: change the `@return` annotation from string to void (and adjust any descriptive text if present); ensure the docblock matches the actual method behavior (the method currently uses bare return;) so no code changes are required beyond updating the PHPDoc.
🧹 Nitpick comments (4)
inc/ui/class-toolbox.php (1)
30-33: Consider:Toolboxis not aBase_Elementsubclass.Unlike other files in this PR,
Toolboxis a standalone class (not extendingBase_Element). The return type removal is harmless, but it may not have been necessary for addon compatibility if this class isn't typically extended by addons. The retained PHPDoc ensures documentation clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@inc/ui/class-toolbox.php` around lines 30 - 33, The init method in the Toolbox class lost its return type while Toolbox is a standalone class (not extending Base_Element); to keep API consistency with other elements restore the explicit return type (e.g., : void) on Toolbox::init so its signature matches the pattern used elsewhere and preserves addon compatibility and documentation clarity.inc/integrations/host-providers/class-cpanel-host-provider.php (2)
265-268: Remaining return type onget_site_url()method.The
get_site_url()method still has: stringreturn type declaration. While this may be less likely to be overridden, for consistency with the coding guidelines it should be removed.Suggested fix
- public function get_site_url($site_id = null): string { + public function get_site_url($site_id = null) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@inc/integrations/host-providers/class-cpanel-host-provider.php` around lines 265 - 268, The get_site_url method has a concrete return type declaration ": string" which should be removed to follow the project's coding guidelines; update the method signature for get_site_url($site_id = null) by removing the ": string" return type and keep the existing body (return trim(preg_replace(...), '/')); ensure any related docblock (if present) reflects the relaxed return type or is left unchanged to match other provider implementations.
100-103: Remaining return type ondetect()method.The
detect()method still has: boolreturn type declaration. Per the coding guidelines forinc/integrations/**/*.php, return type declarations should be removed from public methods on classes that extend base/abstract classes to prevent fatal errors in addons that may override this method.Suggested fix
- public function detect(): bool { + public function detect() {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@inc/integrations/host-providers/class-cpanel-host-provider.php` around lines 100 - 103, The public method detect() in the Cpanel_Host_Provider class currently declares a return type (: bool); remove the return type declaration from the public function signature so it matches the base/abstract class expectations and avoids fatal overrides in addons—locate the public function detect() declaration and change it to a plain public function detect() without the ": bool" type hint, keeping the existing implementation and return statements intact.inc/integrations/host-providers/class-cloudflare-host-provider.php (1)
147-155: Remaining return type ondetect()method.Same as in
CPanel_Host_Provider, thedetect()method still has: boolreturn type declaration. For consistency with the coding guidelines and other changes in this PR, it should be removed.Suggested fix
- public function detect(): bool { + public function detect() {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@inc/integrations/host-providers/class-cloudflare-host-provider.php` around lines 147 - 155, The detect() method in the Cloudflare host provider still has a PHP return type declaration (: bool); remove the type hint from the detect() method signature in class Cloudflare_Host_Provider (the public function detect() declaration) so it matches the other providers and project coding guidelines, leaving the method body and return value (return false;) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@AGENTS.md`:
- Around line 89-95: Update the "**Type hints**" bullet in AGENTS.md so the
scope matches the actual repository guideline: change the narrowed reference to
`inc/checkout/signup-fields/` back to the broader `inc/checkout/**/*.php` and
explicitly list the full pattern set `{inc/gateways/**/*.php,
inc/ui/class-base-element.php, inc/models/class-base-model.php,
inc/integrations/**/*.php, inc/checkout/**/*.php}` (or an equivalent sentence)
and reiterate the rule: NEVER add PHP return type declarations to public methods
on base/abstract classes or interfaces; use `@return` PHPDoc tags instead, while
private and final methods may use return types.
In `@inc/gateways/class-base-gateway.php`:
- Line 173: The public gateway base methods lost their parameter type hints
which breaks addon gateway overrides; restore the original parameter type
declarations on public methods (e.g., set_order) and the other listed public
methods so child classes can declare narrower types (for example int
$payment_id, Membership $membership) while keeping return types flexible; update
the method signatures in class-base-gateway.php to include the appropriate
parameter type hints that match existing addon expectations and PHP 7.4
contravariance rules, but avoid adding stricter return types that could break
overrides.
In `@inc/integrations/interface-capability-module.php`:
- Line 59: The interface method supports() was changed to an untyped parameter
which breaks implementations that already declare supports(string $feature);
revert the interface signature to declare the parameter as string: update the
supports declaration to public function supports(string $feature); so existing
implementations remain declaration-compatible with the interface; ensure the
change targets the supports() method in the interface definition and run
unit/static checks to confirm no other contravariance issues.
---
Outside diff comments:
In `@inc/ui/class-billing-info-element.php`:
- Around line 267-272: Update the PHPDoc for the method output() in class
Billing_Info_Element (the output method) to reflect that it does not return a
string but renders via side effects: change the `@return` annotation from string
to void (and adjust any descriptive text if present); ensure the docblock
matches the actual method behavior (the method currently uses bare return;) so
no code changes are required beyond updating the PHPDoc.
---
Nitpick comments:
In `@inc/integrations/host-providers/class-cloudflare-host-provider.php`:
- Around line 147-155: The detect() method in the Cloudflare host provider still
has a PHP return type declaration (: bool); remove the type hint from the
detect() method signature in class Cloudflare_Host_Provider (the public function
detect() declaration) so it matches the other providers and project coding
guidelines, leaving the method body and return value (return false;) unchanged.
In `@inc/integrations/host-providers/class-cpanel-host-provider.php`:
- Around line 265-268: The get_site_url method has a concrete return type
declaration ": string" which should be removed to follow the project's coding
guidelines; update the method signature for get_site_url($site_id = null) by
removing the ": string" return type and keep the existing body (return
trim(preg_replace(...), '/')); ensure any related docblock (if present) reflects
the relaxed return type or is left unchanged to match other provider
implementations.
- Around line 100-103: The public method detect() in the Cpanel_Host_Provider
class currently declares a return type (: bool); remove the return type
declaration from the public function signature so it matches the base/abstract
class expectations and avoids fatal overrides in addons—locate the public
function detect() declaration and change it to a plain public function detect()
without the ": bool" type hint, keeping the existing implementation and return
statements intact.
In `@inc/ui/class-toolbox.php`:
- Around line 30-33: The init method in the Toolbox class lost its return type
while Toolbox is a standalone class (not extending Base_Element); to keep API
consistency with other elements restore the explicit return type (e.g., : void)
on Toolbox::init so its signature matches the pattern used elsewhere and
preserves addon compatibility and documentation clarity.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 981b2d4e-a6e2-476f-af7f-a613e07fc399
📒 Files selected for processing (37)
AGENTS.mdinc/checkout/signup-fields/class-base-signup-field.phpinc/gateways/class-base-gateway.phpinc/gateways/class-base-paypal-gateway.phpinc/gateways/class-base-stripe-gateway.phpinc/gateways/class-paypal-gateway.phpinc/gateways/class-paypal-rest-gateway.phpinc/integrations/class-base-capability-module.phpinc/integrations/host-providers/class-base-host-provider.phpinc/integrations/host-providers/class-cloudflare-host-provider.phpinc/integrations/host-providers/class-cpanel-host-provider.phpinc/integrations/host-providers/class-hestia-host-provider.phpinc/integrations/host-providers/interface-dns-provider.phpinc/integrations/interface-capability-module.phpinc/models/class-base-model.phpinc/ui/class-account-summary-element.phpinc/ui/class-base-element.phpinc/ui/class-billing-info-element.phpinc/ui/class-checkout-element.phpinc/ui/class-command-palette-manager.phpinc/ui/class-current-membership-element.phpinc/ui/class-current-site-element.phpinc/ui/class-domain-mapping-element.phpinc/ui/class-invoices-element.phpinc/ui/class-limits-element.phpinc/ui/class-login-form-element.phpinc/ui/class-magic-link-url-element.phpinc/ui/class-my-sites-element.phpinc/ui/class-payment-methods-element.phpinc/ui/class-simple-text-element.phpinc/ui/class-site-actions-element.phpinc/ui/class-site-maintenance-element.phpinc/ui/class-template-previewer.phpinc/ui/class-template-switching-element.phpinc/ui/class-thank-you-element.phpinc/ui/class-toolbox.phpinc/ui/class-tours.php
| - **Type hints**: Use where present in existing code. **NEVER add PHP return type | ||
| declarations (`: void`, `: string`, `: bool`, etc.) to public methods on base/abstract | ||
| classes or interfaces** — external addons extend these classes and PHP will fatal if | ||
| the child class doesn't declare the same return type. Use `@return` PHPDoc tags instead. | ||
| This applies to all classes in `inc/gateways/`, `inc/ui/class-base-element.php`, | ||
| `inc/models/class-base-model.php`, `inc/integrations/`, and `inc/checkout/signup-fields/`. | ||
| Private and final methods may use PHP return types freely. |
There was a problem hiding this comment.
Document the checkout scope consistently.
This narrows the rule to inc/checkout/signup-fields/, but the repository guideline still applies it to all of inc/checkout/**/*.php. Leaving AGENTS.md narrower than the actual rule will mislead future checkout changes.
As per coding guidelines {inc/gateways/**/*.php,inc/ui/class-base-element.php,inc/models/class-base-model.php,inc/integrations/**/*.php,inc/checkout/**/*.php}: NEVER add PHP return type declarations (...) to public methods on base/abstract classes or interfaces.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@AGENTS.md` around lines 89 - 95, Update the "**Type hints**" bullet in
AGENTS.md so the scope matches the actual repository guideline: change the
narrowed reference to `inc/checkout/signup-fields/` back to the broader
`inc/checkout/**/*.php` and explicitly list the full pattern set
`{inc/gateways/**/*.php, inc/ui/class-base-element.php,
inc/models/class-base-model.php, inc/integrations/**/*.php,
inc/checkout/**/*.php}` (or an equivalent sentence) and reiterate the rule:
NEVER add PHP return type declarations to public methods on base/abstract
classes or interfaces; use `@return` PHPDoc tags instead, while private and final
methods may use return types.
| * @return void | ||
| */ | ||
| public function set_order($order): void { | ||
| public function set_order($order) { |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In PHP 7.4+, if a parent class method is declared without a parameter type, can a child override it with narrower typed parameters such as int $payment_id, string $gateway_payment_id, or Membership $membership? Does this cause a fatal method signature compatibility error?
💡 Result:
No, in PHP 7.4+, a child class cannot override a parent class method that has no parameter type with a narrower typed parameter like int $payment_id, string $gateway_payment_id, or Membership $membership. This causes a fatal error: "Declaration of Child::method(int $param) must be compatible with Parent::method($param)". Untyped parameters in PHP are treated as accepting any type (effectively mixed). Adding a specific type in the child makes the parameter more specific (narrower), which violates contravariance rules for parameters. Child methods must have parameter types that are less specific or equal to the parent's (or untyped). This enforces the Liskov Substitution Principle: code expecting to pass any value to the parent method must still work when using the child class instance. Prior to PHP 7.4, it raised a warning; since PHP 7.4 with full contravariance support, incompatible signatures are fatal errors (strictly enforced in PHP 8+). Workarounds: runtime type checks inside the child method, or redesign with interfaces/abstract methods using the desired types.
Citations:
- 1: https://php.net/manual/en/language.oop5.variance.php
- 2: https://wiki.php.net/rfc/covariant-returns-and-contravariant-parameters
- 3: https://php7explained.com/4.2.5-inheritance.html
- 4: https://stackoverflow.com/questions/62930305/how-to-define-stricter-typing-in-child-class-inherited-methods-in-php
- 5: https://stackoverflow.com/questions/65494457/php-8-method-overriding-with-different-types-from-the-same-baseclass
- 6: https://phpstan.org/error-identifiers/method.childParameterType
- 7: Method parameter with no type wrongly reported in child class phpstan/phpstan#9130
Restore parameter types on public gateway base methods.
Removing parameter types from parent methods breaks addon gateway overrides. In PHP 7.4+, if a parent method parameter is untyped, child overrides cannot narrow it with specific types (e.g., int $payment_id, Membership $membership). This violates contravariance rules and causes fatal "Declaration must be compatible" errors when addon gateways load. Return-type removal is safe for extensibility, but parameter-type removal recreates the BC problem on the parameter side.
Affects lines: 173, 217, 597, 616, 631, 646, 824, 861, 873, 885, 899
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@inc/gateways/class-base-gateway.php` at line 173, The public gateway base
methods lost their parameter type hints which breaks addon gateway overrides;
restore the original parameter type declarations on public methods (e.g.,
set_order) and the other listed public methods so child classes can declare
narrower types (for example int $payment_id, Membership $membership) while
keeping return types flexible; update the method signatures in
class-base-gateway.php to include the appropriate parameter type hints that
match existing addon expectations and PHP 7.4 contravariance rules, but avoid
adding stricter return types that could break overrides.
| * @return bool | ||
| */ | ||
| public function supports(string $feature): bool; | ||
| public function supports($feature); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In PHP 7.4+, if an interface declares public function supports($feature);, can an implementing class declare public function supports(string $feature);? How do parameter contravariance rules affect interface method compatibility?
💡 Result:
No, in PHP 7.4+, if an interface declares public function supports($feature); (no type), an implementing class cannot declare public function supports(string $feature); (adding a more specific string type). This violates parameter contravariance rules, which only allow the implementing/overriding method to use a less specific (supertype or untyped) parameter type than the interface/parent, not more specific. Parameter contravariance in PHP 7.4+ permits widening parameter types (e.g., interface string $x -> class object $x or untyped $x), ensuring Liskov Substitution Principle (LSP): code expecting the interface can pass any argument valid for the interface, and the class method accepts it. Narrowing (e.g., no-type -> string) breaks LSP, as code passing a non-string (valid for untyped interface) would fail in the class. This applies to interfaces: the class must provide a compatible signature per PHP manual on interfaces and variance rules. Pre-7.4, parameters were strictly invariant (exact match required, except 7.2+ untyping). Examples confirming prohibition include Stack Overflow discussions on attempting stricter types in implementations, resulting in fatal errors.
Citations:
- 1: https://www.php.net/manual/en/language.oop5.variance.php
- 2: https://stackoverflow.com/questions/66172016/php-type-hint-a-more-specific-type-when-implementing-an-interface
- 3: https://www.php.net/manual/en/language.oop5.interfaces.php
- 4: https://wiki.php.net/rfc/covariant-returns-and-contravariant-parameters
- 5: https://stackoverflow.com/questions/33117185/php-interface-accepts-interface-argument
- 6: https://stackoverflow.com/questions/40245928/type-hinted-interface-methods-not-accepting-derivative-types
- 7: https://php.net/manual/en/language.oop5.variance.php
- 8: https://laravel.io/articles/covariance-and-contravariance-in-php
Keep supports() typed as string.
Removing the parameter type breaks backwards compatibility. In PHP, when an interface declares public function supports($feature) (untyped), any addon implementing public function supports(string $feature) becomes declaration-incompatible—parameter contravariance rules disallow narrowing parameter types in implementations. Existing addons may already have typed implementations based on the current interface signature, and this change will cause fatal errors in those addons when the plugin is updated.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@inc/integrations/interface-capability-module.php` at line 59, The interface
method supports() was changed to an untyped parameter which breaks
implementations that already declare supports(string $feature); revert the
interface signature to declare the parameter as string: update the supports
declaration to public function supports(string $feature); so existing
implementations remain declaration-compatible with the interface; ensure the
change targets the supports() method in the interface definition and run
unit/static checks to confirm no other contravariance issues.
|
Completed via PR #781, merged to main. Merged by deterministic merge pass (pulse-wrapper.sh). Neither MERGE_SUMMARY comment nor PR body text was available. aidevops.sh v3.6.235 spent 12m on this as a headless bash routine. |
- Fix SVN deploy: add || true to grep pipelines under pipefail - Add error reporting on svn commit failure - Update changelog to include PR #781 (return type compat)
…turn type issues Restores parameter type declarations that were incorrectly removed in PR #781: - Base_Gateway::verify_and_complete_payment() — restore int $payment_id - Base_Gateway::redirect_with_error() — restore string $message - Capability_Module::supports() — restore string $feature PHP contravariance rule: if a parent/interface has untyped parameters, child classes cannot declare narrower types (e.g. int) without a fatal 'Declaration must be compatible' error. Keeping types in the parent allows addons to use matching types safely. Also removes remaining missed return type declarations from extensible classes: - CPanel_Host_Provider::detect() — remove : bool - CPanel_Host_Provider::get_site_url() — remove : string - Cloudflare_Host_Provider::detect() — remove : bool PHPDoc fix: - Billing_Info_Element::output() — @return void (was incorrectly @return string) Documentation fix (AGENTS.md): - Expand scope from inc/checkout/signup-fields/ to inc/checkout/ to match the actual guideline (all checkout classes, not just signup-fields subdir) Closes #809
…turn type issues (#816) Restores parameter type declarations that were incorrectly removed in PR #781: - Base_Gateway::verify_and_complete_payment() — restore int $payment_id - Base_Gateway::redirect_with_error() — restore string $message - Capability_Module::supports() — restore string $feature PHP contravariance rule: if a parent/interface has untyped parameters, child classes cannot declare narrower types (e.g. int) without a fatal 'Declaration must be compatible' error. Keeping types in the parent allows addons to use matching types safely. Also removes remaining missed return type declarations from extensible classes: - CPanel_Host_Provider::detect() — remove : bool - CPanel_Host_Provider::get_site_url() — remove : string - Cloudflare_Host_Provider::detect() — remove : bool PHPDoc fix: - Billing_Info_Element::output() — @return void (was incorrectly @return string) Documentation fix (AGENTS.md): - Expand scope from inc/checkout/signup-fields/ to inc/checkout/ to match the actual guideline (all checkout classes, not just signup-fields subdir) Closes #809
1 participant
Summary by CodeRabbit