WordPress.org

Make WordPress Core

Opened 5 months ago

Closed 3 months ago

#45352 closed defect (bug) (fixed)

wp_targeted_link_rel filter can result in rel attribute with no value

Reported by: spartank Owned by: peterwilsoncc
Milestone: 5.1 Priority: normal
Severity: normal Version: 5.1
Component: Formatting Keywords: good-first-bug has-patch has-unit-tests
Focuses: Cc:

Description

The Problem

A function for filter wp_targeted_link_rel that returns an empty string will result in a rel attribute without a value. It would be better in that case to not insert the rel attribute.

How To Reproduce

Create function attached to wp_targeted_link_rel that returns empty string.

function my_targeted_link_rel($rel_values) {
        return '';
}
add_filter('wp_targeted_link_rel', 'my_targeted_link_rel');

Resulting link will contain rel=""

<a href="https://www.google.com" target="_blank" rel="">Link</a>

Possible Solutions

Return original link from wp_targeted_link_rel_callback after applying wp_targeted_link_rel if $rel variable is empty.

Attachments (4)

45352.diff (829 bytes) - added by mcmwebsol 5 months ago.
patch for 45352
45352.2.diff (2.1 KB) - added by meatman89fs 5 months ago.
45352.3.diff (1.5 KB) - added by peterwilsoncc 3 months ago.
45352.4.diff (1.6 KB) - added by peterwilsoncc 3 months ago.

Download all attachments as: .zip

Change History (17)

#1 @SergeyBiryukov
5 months ago

  • Component changed from General to Formatting
  • Keywords needs-patch needs-unit-tests added
  • Milestone changed from Awaiting Review to Future Release

#2 @SergeyBiryukov
5 months ago

  • Keywords good-first-bug added

@mcmwebsol
5 months ago

patch for 45352

#3 @mcmwebsol
5 months ago

  • Keywords has-patch added; needs-patch removed

#4 @spartank
5 months ago

@mcmwebsol I'm new to contributing to core, so I may be wrong, but I feel like it would be a little simpler to put the change in the wp_targeted_link_rel_callback function, right after the apply_filters call.

function wp_targeted_link_rel_callback( $matches ) {
        $link_html = $matches[1];
        $rel_match = array();

        /**
         * Filters the rel values that are added to links with `target` attribute.
         *
         * @since 5.0.0
         *
         * @param string The rel values.
         * @param string $link_html The matched content of the link tag including all HTML attributes.
         */
        $rel = apply_filters( 'wp_targeted_link_rel', 'noopener noreferrer', $link_html );

//New If Statement      
//------------------------------
        if ( ! $rel ) {
                return $matches[0];
        }
//------------------------------
//The rest of the function as is

#5 @pento
5 months ago

  • Milestone changed from Future Release to 5.1

[42770] is being reverted from the 5.0 branch, so I'm moving this ticket to 5.1, so it can be addressed there, instead.

#6 @pento
5 months ago

  • Version changed from 5.0 to trunk

@meatman89fs
5 months ago

#7 @meatman89fs
5 months ago

  • Keywords has-unit-tests added; needs-unit-tests removed

#8 @SergeyBiryukov
5 months ago

  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

#9 @SergeyBiryukov
5 months ago

45352.2.diff looks good at a glance.

filter_empty_targeted_link_rel() can be replaced with the existing __return_empty_string() function.

#10 @spartank
5 months ago

The proposed change makes it impossible to use a filter to not set a value for rel on links that open in a new tab. Why shouldn't this be possible as opposed to falling back to "noreferrer noopener" if the filter returns an empty string?

Shouldn't the test case be:

public function test_filter_non_empty_value() {
   add_filter('wp_targeted_link_rel', array( $this, 'filter_empty_targeted_link_rel' ) );
   $content  = '<p>Links: <a href="/" target="_blank">No rel</a></p>';
   $this->assertEquals( $content, wp_targeted_link_rel( $content ) );
   remove_filter( 'wp_targeted_link_rel', array( $this, 'filter_empty_targeted_link_rel' ) );
}

#11 @peterwilsoncc
3 months ago

I'd rather take the approach that if a developer clears wp_targeted_link_rel they don't wish to use the feature. 45352.3.diff returns early if that is the case.

@spartank to answer your question about the test: filters (along with actions and a few other globals) are reset by the test suite automatically between tests so there is no need for the remove_filter() call.

#12 @peterwilsoncc
3 months ago

  • Owner changed from SergeyBiryukov to peterwilsoncc
  • Status changed from reviewing to accepted

45352.4.diff removes the unneeded empty() call.

#13 @peterwilsoncc
3 months ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 44691:

Formatting: Prevent wp_targeted_link_rel() adding an empty attribute.

Bypass adding a rel attribute when the wp_targeted_link_rel filter returns an empty string or other falsy result.

Props mcmwebsol, spartank, meatman89fs.
Fixes #45352.

Note: See TracTickets for help on using tickets.