-
Notifications
You must be signed in to change notification settings - Fork 843
Forms: removes a php warning on forms where the $page global is set to a non integer #46141
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: trunk
Are you sure you want to change the base?
Conversation
Ensures the page number defaults to 1 if $page is not an integer when computing the contact form's ID, preventing potential issues with invalid page values.
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR addresses a PHP warning that occurs when themes overwrite the $page global variable with non-integer values. The fix adds type checking in the Contact_Form constructor to ensure only integer values are passed to the compute_id method, defaulting to 1 for non-integer values.
Key Changes:
- Adds type validation for the
$pageglobal before passing it tocompute_id() - Documents the fix in the changelog
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
projects/packages/forms/src/contact-form/class-contact-form.php |
Adds is_int() check to sanitize $page global before use in form ID computation |
projects/packages/forms/changelog/fix-php-warning-on-forms |
Changelog entry documenting the fix for PHP warning with overwritten globals |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| if ( $set_id ) { | ||
| $attributes['id'] = self::compute_id( $attributes, $this->current_post, $page ); | ||
| $page_number = is_int( $page ) ? $page : 1; |
Copilot
AI
Nov 29, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The is_int() check is too strict and may incorrectly default valid numeric string values to 1. In WordPress, the $page global can be a numeric string (e.g., '2') rather than a strict integer type. Consider using is_numeric() or applying intval() directly with a null check instead:
$page_number = ( $page && is_numeric( $page ) ) ? (int) $page : 1;or more simply:
$page_number = $page ? intval( $page ) : 1;Since compute_id() already uses intval() on line 479, the second approach would be sufficient and more permissive for legitimate numeric values.
| $page_number = is_int( $page ) ? $page : 1; | |
| $page_number = $page ? intval( $page ) : 1; |
| $page_number = is_int( $page ) ? $page : 1; | ||
| $attributes['id'] = self::compute_id( $attributes, $this->current_post, $page_number ); |
Copilot
AI
Nov 29, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fix for handling non-integer $page global values lacks test coverage. Consider adding a test that verifies the constructor handles non-integer $page values correctly (e.g., string values, null, objects). This would prevent regressions and validate the fix. Example test case:
public function test_constructor_handles_non_integer_page_global() {
global $page;
$page = 'not-an-integer'; // Simulating theme overwriting $page
$attributes = array( 'to' => 'test@example.com' );
$form = new Contact_Form( $attributes );
// Verify no warnings and form is created successfully
$this->assertInstanceOf( Contact_Form::class, $form );
}
Code Coverage SummaryCoverage changed in 1 file.
|
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Some themes overwrite the $page global.
Which should be in integer. We convert it to an interger before trying to use it.
Proposed changes:
Other information:
Jetpack product discussion
See p1764374805068119-slack-C06QF6JJDTM
Does this pull request change what data or activity we track or use?
No
Testing instructions:
Do the tests pass?
Load a page with the form.
Load submit the form. Notice that it produces PHP Warnings.