-
Notifications
You must be signed in to change notification settings - Fork 819
Jetpack_Media_Summary: Prevent warnings related to undefined keys and unexpected array offsets #43641
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
…ng unexpected array offsets.
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! Jetpack plugin: The Jetpack plugin has different release cadences depending on the platform:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
Code Coverage SummaryCoverage changed in 1 file.
|
|
||
if ( $number_of_paragraphs <= 2 && is_countable( $extract['image'] ) && 1 === count( $extract['image'] ) ) { | ||
if ( $number_of_paragraphs <= 2 && is_countable( $extract['image'] ) && 1 === count( $extract['image'] ) ) { // @phan-suppress-current-line PhanTypePossiblyInvalidDimOffset -- We established the image offset exists with '! empty( $extract['has']['image']' earlier. |
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 line was flagged by Phan due to the 'image' offset possibly being invalid. However as mentioned when suppressing, we established that the image offset exists earlier.
Previous change was added here, so I've opted to leave as-is: https://github.com/Automattic/jetpack/pull/31250/files#diff-8a52a367cf42b78b5fea4ec178b867663dd4979986a114b95b38fc0463fa73d0
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.
! empty( $extract['has']['image']
Might be missing the obvious here but where is this check? I couldn't find it
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.
We check that here:
} elseif ( ! empty( $extract['has']['image'] ) ) { |
…ier, as it is unrelated and causing issues.
$return['secure']['image'] = self::ssl_img( $return['image'] ); | ||
++$return['count']['image']; | ||
if ( isset( $extract['image'][0]['url'] ) ) { | ||
$return['image'] = $extract['image'][0]['url']; |
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.
I wonder if consumers of this method always expect the image
key to be present. For example enhanced_og_image
here would produce Undefined array key
Warnings
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.
Thanks for digging in to this! Good point - in this case the default is set here:
'image' => '', |
Fixes VULCAN-139
Proposed changes:
Warning: Undefined array key "id"
,Trying to access array offset on null
,Trying to access array offset on value of type null
,Trying to access array offset on false
,Trying to access array offset on value of type bool
Logs: 796d1002405c80e7d4b6965a0c522e14-logstash
Other information:
Jetpack product discussion
pdWQjU-1g0-p2
Does this pull request change what data or activity we track or use?
No.
Testing instructions:
To replicate the most prominent warnings:
On trunk or the latest Jetpack version on self-hosted, WoA or a Simple site:
Compose using shortcodes to embed media from popular sites
is enabled at/wp-admin/admin.php?page=jetpack#writing
(I believe this may be default on Simple sites)[vimeo]
.tail -f /tmp/php-errors
on Simple, or error logs - you should see the following warnings:Warning: Undefined array key "id"
andTrying to access array offset on value of type null
. On Simple test sites, that warning isTrying to access array offset on null
.To test the fix:
For the additional warnings, I've been unable to test, however the added check (see line 268) prevents the warnings and also ensure the relevant URLs are not created - with the previous warnings those URLs would not have been created anyway, so this shouldn't cause issues.
However you can certainly test the Happy Path here - add a new image via an Image block, and there should be no issues. Check 'og' tags in the page source, and you should see 'og' tags for the image URLs. You wouldn't see these if there you commented out
$return['image']
and$return['secure']['image']
in the code.