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

Allow opting-out of AMP-to-AMP links, ensure 'Exit Reader Mode' link is non-AMP #4146

Merged
merged 20 commits into from
Jan 24, 2020
Merged

Allow opting-out of AMP-to-AMP links, ensure 'Exit Reader Mode' link is non-AMP #4146

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
92d0f28
Allow opting-out of AMP-to-AMP links with rel=not-amphtml
kienstra Jan 21, 2020
3feb885
Ensure the 'Exit Reader Mode' link goes to non-AMP
kienstra Jan 21, 2020
10f423a
Add a filter to opt-out of AMP-to-AMP links
kienstra Jan 21, 2020
9409786
Simplify a conditional to use an elseif block
kienstra Jan 21, 2020
404455d
Add some comments in unit test
kienstra Jan 21, 2020
64ddd3e
Remove needless variable, move into conditional
kienstra Jan 21, 2020
25b02fb
Rename filter amp_to_amp_excluded_links
kienstra Jan 21, 2020
8a2e3da
Handle cases of multpile rel values, using Weston's snippets
kienstra Jan 21, 2020
640834f
Update includes/sanitizers/class-amp-link-sanitizer.php
kienstra Jan 21, 2020
3b58df0
Address failed Travis build: PHPCS array_search() warning
kienstra Jan 21, 2020
ffec7e5
Update unit tests for new rel value
kienstra Jan 22, 2020
ff931c3
Commit Weston's suggestion for the Reader mode template
kienstra Jan 23, 2020
77118d7
Change DocBlock of 'amp_to_amp_excluded_links'
kienstra Jan 23, 2020
d953a27
Change filter name to 'amp_to_amp_excluded_urls'
kienstra Jan 23, 2020
b6dda38
Rename parameter 'excluded_amp_links' to 'excluded_urls'
kienstra Jan 23, 2020
f85e8be
Handle case of something like #heading at end of URL
kienstra Jan 23, 2020
a6d1597
Commit Weston's suggestion to prevent conditional in Standard mode
kienstra Jan 23, 2020
160eb87
Add a since tag to the new filter amp_to_amp_excluded_urls
kienstra Jan 23, 2020
4701234
Align => operators, from a recent commit
kienstra Jan 23, 2020
101b38d
Fix some phpcs errors, mainly => alignment
kienstra Jan 23, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 20 additions & 4 deletions includes/amp-helper-functions.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -928,10 +928,26 @@ function amp_get_content_sanitizers( $post = null ) {
$sanitizers['AMP_Nav_Menu_Dropdown_Sanitizer'] = $theme_support_args['nav_menu_dropdown'];
}

if ( $amp_to_amp_linking_enabled ) {
$sanitizers['AMP_Link_Sanitizer'] = [
'paired' => ! amp_is_canonical(),
];
if ( $amp_to_amp_linking_enabled && AMP_Theme_Support::STANDARD_MODE_SLUG !== AMP_Theme_Support::get_support_mode() ) {

/**
* Filters the list of URLs which are excluded from being included in AMP-to-AMP linking.
*
* This only applies when the amp_to_amp_linking_enabled filter returns true,
* which it does by default in Transitional mode. This filter can be used to opt-in
* when in Reader mode. This does not apply in Standard mode.
* Only frontend URLs on the frontend need be excluded, as all other URLs are never made into AMP links.
*
* @since 1.5.0
*
westonruter marked this conversation as resolved.
Show resolved Hide resolved
* @param string[] The URLs to exclude from having AMP-to-AMP links.
*/
$excluded_urls = apply_filters( 'amp_to_amp_excluded_urls', [] );

$sanitizers['AMP_Link_Sanitizer'] = array_merge(
[ 'paired' => ! amp_is_canonical() ],
compact( 'excluded_urls' )
);
}

/**
Expand Down
46 changes: 39 additions & 7 deletions includes/sanitizers/class-amp-link-sanitizer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,14 +31,32 @@ class AMP_Link_Sanitizer extends AMP_Base_Sanitizer {
*/
const DEFAULT_META_CONTENT = 'AMP-Redirect-To; AMP.navigateTo';

/**
* The rel attribute value for AMP links.
*
* @var string
*/
const REL_VALUE_AMP = 'amphtml';

/**
* The rel attribute value that will force non-AMP links.
*
* Normally, in a paired mode, links to the same origin will be for AMP.
* But by adding this rel value, the link will be to non-AMP.
*
* @var string
*/
const REL_VALUE_NON_AMP_TO_AMP = 'noamphtml';

/**
* Placeholder for default arguments, to be set in child classes.
*
* @var array
*/
protected $DEFAULT_ARGS = [ // phpcs:ignore WordPress.NamingConventions.ValidVariableName.PropertyNotSnakeCase
'paired' => false, // Only set to true when in a paired mode (will be false when amp_is_canonical()). Controls whether query var is added.
'meta_content' => self::DEFAULT_META_CONTENT,
'paired' => false, // Only set to true when in a paired mode (will be false when amp_is_canonical()). Controls whether query var is added.
'meta_content' => self::DEFAULT_META_CONTENT,
'excluded_urls' => [], // URLs in this won't have AMP-to-AMP links in a paired mode.
];

/**
Expand Down Expand Up @@ -132,12 +150,26 @@ public function process_links() {
}

$href = $element->getAttribute( 'href' );

if ( $this->is_frontend_url( $href ) && '#' !== substr( $href, 0, 1 ) ) {
$rel = $element->hasAttribute( 'rel' ) ? array_filter( preg_split( '/\s+/', $element->getAttribute( 'rel' ) ) ) : [];
$pos = array_search( self::REL_VALUE_NON_AMP_TO_AMP, $rel, true );
if ( false !== $pos ) {
// The rel has a value to opt-out of AMP-to-AMP links, so strip it and ensure the link is to non-AMP.
unset( $rel[ $pos ] );
if ( empty( $rel ) ) {
$element->removeAttribute( 'rel' );
} else {
$element->setAttribute( 'rel', implode( ' ', $rel ) );
}
} elseif (
$this->is_frontend_url( $href )
&&
'#' !== substr( $href, 0, 1 )
&&
! in_array( strtok( $href, '#' ), $this->args['excluded_urls'], true )
) {
// Always add the amphtml link relation when linking enabled.
$rel = $element->hasAttribute( 'rel' ) ? $element->getAttribute( 'rel' ) . ' ' : '';
$rel .= 'amphtml';
$element->setAttribute( 'rel', $rel );
array_push( $rel, self::REL_VALUE_AMP );
$element->setAttribute( 'rel', implode( ' ', $rel ) );

// Only add the AMP query var when requested (in Transitional or Reader mode).
if ( ! empty( $this->args['paired'] ) ) {
Expand Down
2 changes: 1 addition & 1 deletion templates/header-bar.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@
<?php $canonical_link_url = $this->get( 'post_canonical_link_url' ); ?>
<?php if ( $canonical_link_url ) : ?>
<?php $canonical_link_text = $this->get( 'post_canonical_link_text' ); ?>
<a class="amp-wp-canonical-link" href="<?php echo esc_url( $canonical_link_url ); ?>">
<a class="amp-wp-canonical-link" rel="noamphtml" href="<?php echo esc_url( $canonical_link_url ); ?>">
<?php echo esc_html( $canonical_link_text ); ?>
</a>
<?php endif; ?>
Expand Down
43 changes: 43 additions & 0 deletions tests/php/test-amp-helper-functions.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -813,6 +813,49 @@ public function test_amp_get_content_sanitizers_deprecated_param() {
amp_get_content_sanitizers( $post );
}

/**
* Test AMP-to-AMP linking.
*
* @covers ::amp_get_content_sanitizers()
*/
public function test_amp_get_content_sanitizers_amp_to_amp() {
$link_sanitizer_class_name = 'AMP_Link_Sanitizer';

// If AMP-to-AMP linking isn't enabled, this sanitizer shouldn't be present.
add_filter( 'amp_to_amp_linking_enabled', '__return_false' );
$sanitizers = amp_get_content_sanitizers();
$this->assertArrayNotHasKey( $link_sanitizer_class_name, $sanitizers );

// Now that AMP-to-AMP linking is enabled, this sanitizer should be present.
add_filter( 'amp_to_amp_linking_enabled', '__return_true' );
$sanitizers = amp_get_content_sanitizers();
$this->assertEquals(
[
'paired' => true,
'excluded_urls' => [],
],
$sanitizers[ $link_sanitizer_class_name ]
);

$excluded_urls = [ 'https://baz.com', 'https://example.com/one' ];
add_filter(
'amp_to_amp_excluded_urls',
static function() use ( $excluded_urls ) {
return $excluded_urls;
}
);

// The excluded URLs passed to the filter should be present in the sanitizer.
$sanitizers = amp_get_content_sanitizers();
$this->assertEquals(
[
'paired' => true,
'excluded_urls' => $excluded_urls,
],
$sanitizers[ $link_sanitizer_class_name ]
);
}

/**
* Test post_supports_amp().
*
Expand Down
82 changes: 63 additions & 19 deletions tests/php/test-class-amp-link-sanitizer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,62 +38,106 @@ public function test_amp_to_amp_navigation( $paired ) {
$wp_rewrite->init();
$wp_rewrite->flush_rules();

$post_to_link_to = self::factory()->post->create(
[
'post_name' => 'link-target-post',
'post_status' => 'publish',
'post_type' => 'post',
]
$post_link = get_permalink(
self::factory()->post->create(
[
'post_name' => 'link-target-post',
'post_status' => 'publish',
'post_type' => 'post',
]
)
);
$excluded_amp_link = get_permalink( self::factory()->post->create() );
$excluded_urls = [ $excluded_amp_link ];

$links = [
'home-link' => [
'home-link' => [
'href' => home_url( '/' ),
'expected_amp' => true,
'expected_rel' => 'amphtml',
],
'internal-link' => [
'href' => get_permalink( $post_to_link_to ),
'internal-link' => [
'href' => $post_link,
'expected_amp' => true,
'expected_rel' => 'amphtml',
],
'ugc-link' => [
'non_amp_to_amp_rel' => [
'href' => $post_link,
'expected_amp' => false,
'rel' => 'noamphtml',
'expected_rel' => null,
],
'two_rel' => [
'href' => $post_link,
'expected_amp' => false,
'rel' => 'help noamphtml',
'expected_rel' => 'help',
],
'multiple_rel' => [
'href' => $post_link,
'expected_amp' => false,
'rel' => 'noamphtml nofollow help',
'expected_rel' => 'nofollow help',
],
'rel_trailing_space' => [
'href' => $post_link,
'expected_amp' => false,
'rel' => 'noamphtml ',
'expected_rel' => null,
],
'excluded_amp_link' => [
'href' => $excluded_amp_link,
'expected_amp' => false,
'expected_rel' => null,
],
'fragment_identifier' => [
'href' => $excluded_amp_link . '#heading',
'expected_amp' => false,
'expected_rel' => null,
],
'ugc-link' => [
'rel' => 'ugc',
'href' => home_url( '/some/user/generated/data/' ),
'expected_amp' => true,
'expected_rel' => 'ugc amphtml',
],
'page-anchor' => [
'page-anchor' => [
'href' => '#top',
'expected_amp' => false,
'expected_rel' => null,
],
'other-page-anchor' => [
'href' => get_permalink( $post_to_link_to ) . '#top',
'other-page-anchor' => [
'href' => $post_link . '#top',
'expected_amp' => true,
'expected_rel' => 'amphtml',
],
'external-link' => [
'external-link' => [
'href' => 'https://external.example.com/',
'expected_amp' => false,
'expected_rel' => null,
],
'non_amp_rel_removed' => [
'href' => 'https://external.example.com/',
'expected_amp' => false,
'rel' => 'noamphtml',
'expected_rel' => null,
],
'php-file-link' => [
'php-file-link' => [
'href' => site_url( '/wp-login.php' ),
'expected_amp' => false,
'expected_rel' => null,
],
'feed-link' => [
'feed-link' => [
'href' => get_feed_link(),
'expected_amp' => false,
'expected_rel' => null,
],
'admin-link' => [
'admin-link' => [
'href' => admin_url( 'options-general.php?page=some-plugin' ),
'expected_amp' => false,
'expected_rel' => null,
],
'image-link' => [
'image-link' => [
'href' => content_url( '/some-image.jpg' ),
'expected_amp' => false,
'expected_rel' => null,
Expand All @@ -116,7 +160,7 @@ public function test_amp_to_amp_navigation( $paired ) {

$dom = AMP_DOM_Utils::get_dom_from_content( $html );

$sanitizer = new AMP_Link_Sanitizer( $dom, compact( 'paired' ) );
$sanitizer = new AMP_Link_Sanitizer( $dom, compact( 'paired', 'excluded_urls' ) );
$sanitizer->sanitize();

// Confirm admin bar is unchanged.
Expand Down