Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Prevent errors in admin bar filters from non-array arguments #4207

Merged
merged 3 commits into from
Jan 31, 2020
Merged

Prevent errors in admin bar filters from non-array arguments #4207

merged 3 commits into from
Jan 31, 2020

Conversation

kienstra
Copy link
Contributor

@kienstra kienstra commented Jan 31, 2020 •
edited
Loading

Summary

Fixes #4145

Checklist

  • My pull request is addressing an open issue (please create one otherwise).
  • My code is tested and passes existing tests.
  • My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

@kienstra
Commit Weston's fix for the in_array() error, add small tests
ff4375a 
Props @westonruter for the fix
Prevents running in_array() if the deps
are not an array.
@googlebot googlebot added the cla: yes Signed the Google CLA label Jan 31, 2020
@kienstra
Copy link
Contributor Author

Hi @westonruter,
No hurry on this, but this is ready for review when you have a chance.

Thanks, Weston!

westonruter
westonruter requested changes Jan 31, 2020
View reviewed changes
tests/php/test-class-amp-theme-support.php Outdated
public function test_filter_admin_bar_style_loader_tag_non_array() {
wp_enqueue_style( 'admin-bar' );
$GLOBALS['wp_styles']->registered['admin-bar']->deps = null;
$tag = 'foo';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be an actual link tag as normally passed in. Otherwise the test is possibly failing silently.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, good point. Applied in 68964a4

tests/php/test-class-amp-theme-support.php Outdated
public function test_filter_admin_bar_script_loader_tag_non_array() {
wp_enqueue_script( 'admin-bar' );
$GLOBALS['wp_scripts']->registered['admin-bar']->deps = null;
$tag = 'example-tag';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above. This should be an actual script tag as is normally passed into the method.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, applied in the same commit, 68964a4

@westonruter westonruter added this to the v1.4.3 milestone Jan 31, 2020
@kienstra
Use actual $tag values passed from filters, though the domains are ch…
68964a4 
…anged

As Weston mentioned, this
ensures the test doesn't give false results.
westonruter
westonruter reviewed Jan 31, 2020
View reviewed changes
tests/php/test-class-amp-theme-support.php
wp_enqueue_script( 'admin-bar' );
$GLOBALS['wp_scripts']->registered['admin-bar']->deps = null;
$tag = '<script src="https://example.com/wp-includes/js/admin-bar.js?ver=5.3.2"></script>';
$this->assertEquals( $tag, AMP_Theme_Support::filter_admin_bar_script_loader_tag( $tag, 'example' ) );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose in reality the assertion here serves the purpose of preventing the test from being flagged for having no assertions. Since there is no style registered with the handle of baz, neither of the conditions self::is_exclusively_dependent( wp_scripts(), $handle, 'admin-bar' ) nor self::has_dependency( wp_scripts(), $handle, 'admin-bar' ) will ever be true.

@westonruter westonruter merged commit 17f3594 into develop Jan 31, 2020
@westonruter westonruter deleted the fix/admin-bar-array branch January 31, 2020 05:50
westonruter pushed a commit that referenced this pull request Jan 31, 2020
@kienstra @westonruter
Prevent errors in admin bar filters from non-array arguments (#4207)
f8acc9a 
Commit Weston's fix for the in_array() error, add small tests

Prevents running in_array() if the deps are not an array.
westonruter added a commit that referenced this pull request Feb 1, 2020
@westonruter
Merge branch 'develop' of github.com:ampproject/amp-wp into enhanceme…
c4194fa 
…nt/4070-validate-props

* 'develop' of github.com:ampproject/amp-wp: (108 commits)
  Update dependency semver to v7.1.2 (#4206)
  Update dependency @babel/core to v7.8.4 (#4199)
  Prevent errors in admin bar filters from non-array arguments (#4207)
  Discontinue preserving original style[amp-custom]
  Update dependency terser-webpack-plugin to v2.3.4 (#4189)
  Update dependency @babel/cli to v7.8.4 (#4198)
  Update dependency browserslist to v4.8.6 (#4196)
  Update dependency dealerdirect/phpcodesniffer-composer-installer to v0.6.2 (#4186)
  Split Reader mode style template part stylesheet from action-supplied styles
  Remove needless $xmlns_value_to_strip variable
  Fix PHPCS error
  Commit Weston's suggestion to use documentElement
  Use $html->hasAttributes to possibly exit early.
  If the html[lang] is "", overwrite it with the xml:lang
  Conditionally remove html[xmlns], and convert html[xml:lang] to html[lang]
  If head[profile=], strip profile, but don't create a new link
  Revert 7f60c5c (If there's already a link[rel=profile], don't create another
  Fix phpcs issues in failed Travis build
  If there's already a link[rel=profile], don't create another
  Convert head[profile] attribute to a link[rel=profile]
  ...
westonruter added a commit that referenced this pull request Feb 13, 2020
@westonruter
Merge tag '1.4.3'
06162df 
* tag '1.4.3': (22 commits)
  Update readme and screenshots for Stories removal (#4259)
  Open story export instructions in a new window (#4258)
  Bump version to 1.4.3-RC1
  Hide Stories options and add deprecation notice (#4219)
  Fix malformed conversion of relative action URLs for forms (#4250)
  Limit Stories experience to WP 5.3 & Gutenberg 7.1.0 (#4217)
  Prevent errors in admin bar filters from non-array arguments (#4207)
  Update @wordpress/e2e-test-utils dependency
  Revert update of mustache/mustache dependency
  Update composer.lock
  Update WP CLI to 2.4.0
  For WordPress.tv embed, Use an oembed filter instead of block filter (#4164)
  Update readme to add FAQs section (#4159)
  Apply workaround to fix test__multiple_valid_image_files (#4034)
  Ignore Story editor tests (#4043)
  Update amp-video embed regex pattern to include other Vimeo URL formats (#4051)
  Update amp-instagram embed regex (#4053)
  Update wp-dev-lib package (#4029)
  Fix conversion of forms with relative action URLs (#4003)
  Improve release instructions (#3995)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@westonruter westonruter westonruter approved these changes

Assignees
No one assigned
Labels
cla: yes Signed the Google CLA
Projects
None yet
Milestone
v1.4.3
Development

Successfully merging this pull request may close these issues.

PHP Warning due to passing null as haystack value for in_array()
3 participants
@kienstra @westonruter @googlebot