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

Extend Sandboxing experiment to Paired AMP modes #7288

Merged
merged 61 commits into from
Nov 30, 2022

Conversation

thelovekesh
Copy link
Collaborator

@thelovekesh thelovekesh commented Oct 7, 2022

Summary

Fixes #7268

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 thelovekesh force-pushed the add/sandboxing-in-all-modes branch from 288cbe9 to 40688e0 Compare November 2, 2022 14:11
@westonruter
Copy link
Member

For this existing code:

/**
* Filters the text to be used in the mobile switcher link.
*
* Use the `amp_is_request()` function to determine whether you are filtering the
* text for the link to go to the non-AMP version or the AMP version.
*
* @since 2.0
*
* @param string $text Link text to display.
*/
$text = apply_filters( 'amp_mobile_version_switcher_link_text', $text );

It should probably afterward do:

if ( empty( $text ) ) {
    return;
}

This would allow someone to turn off the link via:

add_filter( 'amp_mobile_version_switcher_styles_used', '__return_false' );
add_filter( 'amp_mobile_version_switcher_link_text', '__return_empty_string' );

@thelovekesh thelovekesh marked this pull request as ready for review November 4, 2022 10:11
@github-actions
Copy link
Contributor

github-actions bot commented Nov 4, 2022

Plugin builds for dbe9350 are ready 🛎️!%0A- Download development build%0A- Download production build

Co-authored-by: Weston Ruter <[email protected]>
@codecov
Copy link

codecov bot commented Nov 22, 2022

Codecov Report

Merging #7288 (8cc7b58) into develop (650a4eb) will increase coverage by 12.94%.
The diff coverage is 97.87%.

❗ Current head 8cc7b58 differs from pull request most recent head 5e87dbf. Consider uploading reports for the commit 5e87dbf to get more accurate results

Impacted file tree graph

@@              Coverage Diff               @@
##             develop    #7288       +/-   ##
==============================================
+ Coverage      64.88%   77.82%   +12.94%     
- Complexity         0     6867     +6867     
==============================================
  Files             67      276      +209     
  Lines           1159    19842    +18683     
==============================================
+ Hits             752    15443    +14691     
- Misses           407     4399     +3992     
Flag Coverage Δ
javascript 64.88% <ø> (ø)
php 78.63% <97.87%> (?)
unit 78.63% <97.87%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/Sandboxing.php 99.00% <75.00%> (ø)
includes/amp-helper-functions.php 83.05% <100.00%> (ø)
...cludes/validation/class-amp-validation-manager.php 83.20% <100.00%> (ø)
src/MobileRedirection.php 86.29% <100.00%> (ø)
src/Optimizer/Transformer/AmpSchemaOrgMetadata.php 85.71% <0.00%> (ø)
src/Instrumentation/EventWithDuration.php 100.00% <0.00%> (ø)
...ludes/embeds/class-amp-wordpress-embed-handler.php 97.36% <0.00%> (ø)
includes/embeds/class-amp-imgur-embed-handler.php 66.66% <0.00%> (ø)
includes/embeds/class-amp-scribd-embed-handler.php 84.21% <0.00%> (ø)
src/PairedUrlStructure.php 100.00% <0.00%> (ø)
... and 203 more

@thelovekesh thelovekesh force-pushed the add/sandboxing-in-all-modes branch from 8cc7b58 to 5e87dbf Compare November 23, 2022 03:42
@thelovekesh thelovekesh force-pushed the add/sandboxing-in-all-modes branch from 8bec181 to 0d2c16f Compare November 24, 2022 05:42
…_used()

Test if it keeps the required AMP markup if AMP elements are present
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.

I was doing some thorough testing and I found two additional required changes. Almost there!

Comment on lines +71 to +73
if ( ! amp_is_canonical() && ( $is_mobile_redirect_enabled || ( 1 === $sandboxing_level || 2 === $sandboxing_level ) ) ) {
add_action( 'wp_head', [ $this, 'add_mobile_alternative_link' ] );
}
Copy link
Member

Choose a reason for hiding this comment

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

Note to self: previously this was done below.

if ( ! amp_is_request() ) {
	add_action( 'wp_head', [ $this, 'add_mobile_alternative_link' ] );
}

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.

🎉

@westonruter westonruter merged commit 6d28f28 into develop Nov 30, 2022
@westonruter westonruter deleted the add/sandboxing-in-all-modes branch November 30, 2022 21:58
@westonruter westonruter modified the milestones: v2.3.1, v2.4 Jan 26, 2023
@westonruter westonruter added the Changelogged Whether the issue/PR has been added to release notes. label Feb 8, 2023
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.

Extend Sandboxing experiment to Paired AMP modes
2 participants