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 filter to exclude links on an individual basis from AMP-to-AMP linking #4913

Closed
westonruter opened this issue Jun 24, 2020 · 1 comment · Fixed by #4796
Closed

Add filter to exclude links on an individual basis from AMP-to-AMP linking #4913

westonruter opened this issue Jun 24, 2020 · 1 comment · Fixed by #4796
Labels
Changelogged Whether the issue/PR has been added to release notes. WS:Core Work stream for Plugin core
Milestone

Comments

@westonruter
Copy link
Member

Feature description

AMP-to-AMP linking was introduced in #1389 via #3627 and enabled by default in Transitional mode or in Reader mode via the amp_to_amp_linking_enabled filter. Then in #3689 via #4146 individual links can be disabled from AMP-to-AMP linking in these ways:

  • Filter amp_to_amp_excluded_urls to add all URLs that should be excluded from AMP.
  • Add a noamphtml link relation to links on the page.

However, this has turned out to not be very convenient. Given existing post content that has a bunch of links in it, it is difficult to use the the_content filter to parse out the links and either add them to amp_to_amp_excluded_urls or inject a noamphtml link relation. In other cases, where the links are not in the_content (e.g. in the template itself), this may not be possible at all, forcing a developer to create a custom sanitizer to inject the noamphtml link relations.

What would be the most convenient is if there was a filter like amp_to_amp_linking_element_excluded which was passed was passed a boolean value to control the exclusion, along with the specific link context. For example:

/**
 * Filters whether AMP-to-AMP is excluded for an element.
 *
 * The element may be either a link (`a` or `area`) or a `form`.
 *
 * @param bool       $excluded Excluded. Default value is whether element already has a `noamphtml` link relation or the URL is among `excluded_urls`.
 * @param string     $url      URL considered for exclusion.
 * @param string[]   $rel      Link relations.
 * @param DOMElement $element  The element considered for excluding from AMP-to-AMP linking. May be instance of `a`, `area`, or `form`.
 */
$excluded = (bool) apply_filters( 'amp_to_amp_linking_element_excluded', $excluded, $url, $rel, $element );

The initial value for $excluded would reflect whether or not $url was among the amp_to_amp_excluded_urls or if the link had a noamphtml link relation. But then a plugin could have fine-grained control over whether a link points to another AMP page. For example, if you wanted to quickly exclude categories from AMP-to-AMP linking, you could do:

add_filter( 'amp_to_amp_linking_element_excluded', function ( $excluded, $url ) {
    if ( false !== strpos( $url, '/category/' ) ) {
        $excluded = true;
    }
    return $excluded;
}, 10, 2 );

At the time AMP-to-AMP linking was introduced, the ability to filter links in this way was not doable since the arguments being passed into the sanitizers needed to deterministically result in a given output, since the sanitizer args were used as the cache key for the “post-processor cache”. This is no longer a concern, however, since the post-processor cache was eliminated in #4391 due to being ineffectual (see #4178). So now we can safely apply_filters() inside of sanitizers.


Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

Implementation brief

QA testing instructions

Demo

Changelog entry

@westonruter westonruter added this to the v1.6 milestone Jun 24, 2020
@westonruter westonruter self-assigned this Jun 24, 2020
westonruter added a commit that referenced this issue Jun 24, 2020
@schlessera
Copy link
Collaborator

QA passed

Given the following filter:

Image 2020-07-15 at 12 51 05 PM

The correct link is excluded from AMP-to-AMP linking:

Image 2020-07-15 at 12 50 53 PM

@westonruter westonruter removed their assignment Jul 16, 2020
@westonruter westonruter added the Changelogged Whether the issue/PR has been added to release notes. label Jul 17, 2020
@kmyram kmyram added the WS:Core Work stream for Plugin core label Aug 5, 2020
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. WS:Core Work stream for Plugin core
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants