Make WordPress Core

Opened 3 months ago

Closed 2 months ago

#63220 closed defect (bug) (fixed)

Documentation for embed_oembed_html misleading

Reported by: apermo's profile apermo Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 6.8 Priority: normal
Severity: normal Version: 2.9
Component: Embeds Keywords: has-patch dev-feedback
Focuses: docs Cc:

Description

<?php
if ( ! empty( $cache ) ) {
        /**
         * Filters the cached oEmbed HTML.
         *
         * @since 2.9.0
         *
         * @see WP_Embed::shortcode()
         *
         * @param string|false $cache   The cached HTML result, stored in post meta.
         * @param string       $url     The attempted embed URL.
         * @param array        $attr    An array of shortcode attributes.
         * @param int          $post_id Post ID.
         */
        return apply_filters( 'embed_oembed_html', $cache, $url, $attr, $post_id );
}
<?php
if ( $html ) {
        /** This filter is documented in wp-includes/class-wp-embed.php */
        return apply_filters( 'embed_oembed_html', $html, $url, $attr, $post_id );
}

While the documentation suggests that $cache/$html can be false or a string, it can't be false and even not an empty string.

This is misleading, we wanted to create a filter for that and added type hints to our filter and were unsure about the type. After reading the code we found that it was misleading.

I will open a PR for this.

Change History (8)

This ticket was mentioned in PR #8640 on WordPress/wordpress-develop by @apermo.


3 months ago
#1

  • Keywords has-patch added

As described in the trac ticket, the PHPdoc for embed_oembed_html is misleading. This fixes this issue.

Trac ticket: https://core.trac.wordpress.org/ticket/63220#ticket

#2 @oglekler
3 months ago

  • Focuses docs added

#3 @SergeyBiryukov
3 months ago

  • Milestone changed from Awaiting Review to 6.9

#4 @SergeyBiryukov
3 months ago

  • Owner set to SergeyBiryukov
  • Resolution set to fixed
  • Status changed from new to closed

In 60127:

Docs: Correct the type of the $cache parameter in embed_oembed_html.

The value cannot be false at this point, as the filter is only applied to non-empty output.

Follow-up to [25726], [46661].

Props apermo, xate.
Fixes #63220.

#5 @peterwilsoncc
3 months ago

  • Keywords dev-feedback added
  • Milestone changed from 6.9 to 6.8
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening and moving to the 6.8 milestone for r60127 to be considered for merging to the 6.8 branch.

#6 @zunaid321
3 months ago

This really reduces confusion and potential misuse, especially for the developers who works on custom embed behavior.

#7 @apermo
3 months ago

That confusion is exactly what lead me and @xate to find this. We were discussing which type hints we needed and since PHPdoc was suggesting that false could be a valid value, we were about to make our code more complicated that it needed to be.
So fixing this, will potentially have developers create less complex code on their side. I'd highly welcome it in 6.8.

Since it is only a PHPdoc fix, it has no potential of breaking the update, I don't see any reason to hold it back till 6.9.
If it is to short of a notice for 6.8, maybe it could be considered for 6.8.1?

#8 @jorbin
2 months ago

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

In 60140:

Docs: Correct the type of the $cache parameter in embed_oembed_html.

The value cannot be false at this point, as the filter is only applied to non-empty output.

Follow-up to [25726], [46661].

Reviewed By peterwilsoncc, jorbin.
Merges [60127] to the 6.8 branch.

Props apermo, xate, SergeyBiryukov.
Fixes #63220.

Note: See TracTickets for help on using tickets.