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

Strip leading BOM+whitespace and trailing HTML comment before parsing validation response JSON #4679

Merged
merged 1 commit into from
May 12, 2020
Merged

Strip leading BOM+whitespace and trailing HTML comment before parsing validation response JSON #4679

merged 1 commit into from
May 12, 2020

Conversation

westonruter
Copy link
Member

@westonruter westonruter commented May 10, 2020 •
edited

Summary

Fixes #4678

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).

@googlebot googlebot added the cla: yes Signed the Google CLA label May 10, 2020
@westonruter
Copy link
Member Author

Build for testing: amp.zip (1.5.4-alpha-20200510T191400Z-314ee33d9)

@amedina amedina self-requested a review May 10, 2020 20:05
amedina
amedina approved these changes May 10, 2020
View reviewed changes
Copy link
Member

@amedina amedina left a comment

Choose a reason for hiding this comment

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

Neat. LGTM.

@westonruter westonruter merged commit 5332ec4 into develop May 12, 2020
@westonruter westonruter deleted the fix/json-parsing-resiliency branch May 12, 2020 06:28
westonruter added a commit that referenced this pull request May 12, 2020
@westonruter
Strip leading BOM and whitespace and trailing HTML comment before par…
279dde0 
…sing validation response JSON (#4679)
schlessera
schlessera reviewed May 12, 2020
View reviewed changes
includes/validation/class-amp-validation-manager.php
@@ -2114,6 +2114,17 @@ public static function validate_url( $url ) {
$validation_url
);

// Strip byte order mark (BOM).
if ( "\xEF\xBB\xBF" === substr( $response, 0, 3 ) ) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would have turned this into a while loop, as under certain circumstances, some systems prepend the BO without checking if there is one already. So you can end up with multiple ones as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. Done in #4683.

westonruter added a commit that referenced this pull request May 12, 2020
@westonruter
Strip multiple BOM characters
6b2991a 
Amends #4679
@westonruter westonruter mentioned this pull request May 12, 2020
Merged
3 tasks
westonruter added a commit that referenced this pull request May 12, 2020
@westonruter
Strip multiple BOM characters (#4683)
7f36afd 
Amends #4679
westonruter added a commit that referenced this pull request May 12, 2020
@westonruter
Strip multiple BOM characters (#4683)
fb42d6a 
Amends #4679
westonruter added a commit that referenced this pull request May 12, 2020
@westonruter
Merge branch 'develop' of github.com:ampproject/amp-wp into add/plugi…
36ceda5 
…n-suppression

* 'develop' of github.com:ampproject/amp-wp:
  Strip multiple BOM characters (#4683)
  Strip leading BOM and whitespace and trailing HTML comment before parsing validation response JSON (#4679)
  Update dependency xwp/wp-dev-lib to v1.6.4
  Update dependency eslint to v7
westonruter added a commit that referenced this pull request May 13, 2020
@westonruter
Merge branch 'develop' of github.com:ampproject/amp-wp into prototype…
6a0271b 
…/reader-mode-themes

* 'develop' of github.com:ampproject/amp-wp: (59 commits)
  Strip multiple BOM characters (#4683)
  Strip leading BOM and whitespace and trailing HTML comment before parsing validation response JSON (#4679)
  Update dependency xwp/wp-dev-lib to v1.6.4
  Update dependency eslint to v7
  Remove unused RuntimeException
  Use explicit string type instead of generic array in return
  Avoid method map and replace with a switch
  Defer removal of attributes until after layout application
  Fix post preview in Reader mode (#4665)
  Ensure that source map comment is preserved at the end of amp-custom.css
  Pass DOMAttr instances rather than attribute name and value as separate args
  Enable SSR by default, remove use of WP_DEBUG as a signal. (#4669)
  Use padding-top and override regular inline style for heights
  Update php-parallel-lint dependency to new name
  Use :first-child to reference sizer in CSS
  Update dependency @wordpress/babel-preset-default to v4.12.1 (#4494)
  Fix error type for bad sizes test
  Provide more precise error for thrown exception
  Test precise error messages
  Fix attribute name in error messages
  ...
westonruter added a commit that referenced this pull request May 15, 2020
@westonruter
Merge branch 'develop' of github.com:ampproject/amp-wp into add/archi…
7743016 
…ve-collection-schema-type

* 'develop' of github.com:ampproject/amp-wp:
  Raise PHPStan level to 4 in lib/common (#4686)
  Update dependency uuid to v8
  Update dependency webpack to v4.43.0
  Update dependency eslint-plugin-jest to v23.11.0
  Update dependency eslint-plugin-react to v7.20.0 (#4691)
  Update dependency postcss to v7.0.30 (#4649)
  Update dependency moment to v2.25.3 (#4639)
  Update dependency babel-jest to v26 (#4652)
  Update dependency uuid to v7.0.3 (#4490)
  Update dependency wp-coding-standards/wpcs to v2.3.0 (#4721)
  Suppress Site Health ICU test if site or home URL is not an IDN (#4698)
  Pin amp-experiment to v0.1 (#4690)
  Strip multiple BOM characters (#4683)
  Strip leading BOM and whitespace and trailing HTML comment before parsing validation response JSON (#4679)
  Disable share icons
  Use amp-embedly-card for Tiktok embeds
  Update dependency xwp/wp-dev-lib to v1.6.4
  Update dependency eslint to v7
  Update dependency terser-webpack-plugin to v2.3.6
@westonruter westonruter added this to the v1.5.4 milestone Jun 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@schlessera schlessera schlessera left review comments

@amedina amedina amedina approved these changes

@pierlon pierlon pierlon approved these changes

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

Successfully merging this pull request may close these issues.

Byte order mark (BOM) breaks ability to do validation
5 participants
@westonruter @schlessera @amedina @pierlon @googlebot