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.

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

Add PWA scripts sanitizer #7141

Merged
merged 15 commits into from
Jun 10, 2022
Merged

Conversation

thelovekesh
Copy link
Collaborator

@thelovekesh thelovekesh commented Jun 8, 2022

Summary

Add PWA scripts sanitizer which adds data-px-verified-tag to the offline/500 pages scripts.

Fixes #7122

Screenshots

Storing offline/500 templates in cache as AMP page with offline page reload scripts.

image

Checklist

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

@thelovekesh
Copy link
Collaborator Author

@westonruter I have created XPath based on the offline script id attribute, which was added in PR - GoogleChromeLabs/pwa-wp#793

One more thing to notice is that if the PWA plugin is not available in the testing environment, the test for this sanitizer will fail due to the unavailability of is_offline and is_500. I thought to add this condition in tests but it doesn't seems much feasible. What are your thoughts in this case?

if (
	! ( function_exists( 'is_offline' ) && is_offline() ) &&
	! ( function_exists( 'is_500' ) && is_500() )
) {
	return;
}

@thelovekesh thelovekesh marked this pull request as ready for review June 9, 2022 07:20
@github-actions
Copy link
Contributor

github-actions bot commented Jun 9, 2022

Plugin builds for 8dc21de are ready 🛎️!

@thelovekesh
Copy link
Collaborator Author

thelovekesh commented Jun 9, 2022

Tests are failing on WP Trunk after adding decoding="async" attribute in image tags in WordPress/wordpress-develop@2702591

@westonruter
Copy link
Member

Tests are failing on WP Trunk after adding decoding="async" attribute in image tags in WordPress/wordpress-develop@2702591

As we discussed, to fix this lets just do the following in the various tests before we do any comparison with the $expected:

$actual = str_replace( ' decoding="async"', '', $actual );

@thelovekesh
Copy link
Collaborator Author

@westonruter I was exploring this scenario and found out that something like this was also added for the loading attribute but instead of hardcoding that a conditional expression is used

$loading_attribute = version_compare( get_bloginfo( 'version' ), '5.5-alpha', '>' ) ? 'loading="lazy"' : '';
return [
'shortcode_with_invalid_id' => [
'[gallery ids=1]',
'',
],
'shortcode_with_valid_ids_in_legacy_mode' => [
'[gallery ids={{id1}},{{id2}},{{id3}}]',
'<style type="text/css"> #gallery-1 { margin: auto; } #gallery-1 .gallery-item { float: left; margin-top: 10px; text-align: center; width: 33%; } #gallery-1 img { border: 2px solid #cfcfcf; } #gallery-1 .gallery-caption { margin-left: 0; } /* see gallery_shortcode() in wp-includes/media.php */ </style>
<amp-carousel width="640" height="480" type="slides" layout="responsive">
<figure class="slide"><a href="{{file_url1}}"><img width="100" height="100" src="{{file1}}.jpg" class="attachment-large size-large" alt="Alt text" ' . $loading_attribute . ' aria-describedby="gallery-1-{{id1}}" layout="fill" object-fit="cover"></a>' . $amp_carousel_caption . '</figure>
<figure class="slide"><a href="{{file_url2}}"><img width="640" height="480" src="{{file2}}.jpg" class="attachment-large size-large" alt="Alt text" ' . $loading_attribute . ' srcset="{{file2}}.jpg 640w, {{file2}}-300x225.jpg 300w" sizes="(max-width: 640px) 100vw, 640px" layout="fill" object-fit="cover"></a></figure>
<figure class="slide"><a href="{{file_url3}}"><img width="100" height="100" src="{{file3}}.jpg" class="attachment-large size-large" alt="Alt text" ' . $loading_attribute . ' layout="fill" object-fit="cover"></a></figure>
</amp-carousel>',

Should I try something similar?

$decoding_attribute   = version_compare( get_bloginfo( 'version' ), '6.0', '>' ) ? 'decoding="async"' : '';

I believe the decoding attribute is going to be introduced in 6.1.

@westonruter
Copy link
Member

Should I try something similar?

That would be better, yes.

Co-authored-by: Weston Ruter <westonruter@google.com>
@thelovekesh thelovekesh marked this pull request as draft June 9, 2022 18:58
@thelovekesh thelovekesh force-pushed the add/pwa-offline-scripts-sanitizer branch 2 times, most recently from e554d82 to 7420960 Compare June 9, 2022 19:31
@thelovekesh thelovekesh force-pushed the add/pwa-offline-scripts-sanitizer branch from 7420960 to 60aad5e Compare June 9, 2022 19:46
@thelovekesh thelovekesh marked this pull request as ready for review June 9, 2022 19:58
@thelovekesh
Copy link
Collaborator Author

@westonruter Now the static analysis is failing. I think this can be because of the new PHPStan release.

With PHPStan version 1.7.11
image

With PHPStan version 1.7.12
image

@thelovekesh thelovekesh self-assigned this Jun 10, 2022
@westonruter westonruter mentioned this pull request Jun 10, 2022
2 tasks
@westonruter
Copy link
Member

Here's how I've been forcing to update my local PHPStan:

rm vendor/bin/phpstan
composer install --no-cache

@westonruter westonruter added this to the v2.3 milestone Jun 10, 2022
@thelovekesh
Copy link
Collaborator Author

thelovekesh commented Jun 10, 2022

Thanks for the info on this step 🙇🏼‍♂️

Copy link
Member

@westonruter westonruter left a comment

Choose a reason for hiding this comment

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

Good work. Thank you.

@westonruter westonruter merged commit e70e6d8 into develop Jun 10, 2022
@westonruter westonruter deleted the add/pwa-offline-scripts-sanitizer branch June 10, 2022 17:27
@westonruter westonruter added the Changelogged Whether the issue/PR has been added to release notes. label Jun 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelogged Whether the issue/PR has been added to release notes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Scripts added by PWA plugin on error template are removed by sanitizer
2 participants