Automattic / jetpack

Security, performance, marketing, and design tools — Jetpack is made by WordPress experts to make WP sites safer and faster, and help you grow your traffic.
https://jetpack.com/
Other
1.59k stars 798 forks source link

Jetpack breaks wp_get_shortlink() instead of removing 'wp_shortlink_wp_head' hook! #8250

Open jsmoriss opened 7 years ago

jsmoriss commented 7 years ago

Jetpack has decided to return an empty string by default when wp_get_shortlink() is called for a custom post type. This disables the "shortlink" meta tag and "Get Shortlink" button, but ALSO breaks the WP core wp_get_shortlink() function! Jetpack is not the only plugin out there -- wp_get_shortlink() is used by other plugins for a variety of reasons -- standard core functionality should not be broken on purpose. Instead of break the wp_get_shortlink() function, please use this instead:

remove_action( 'wp_head', 'wp_shortlink_wp_head' );

Thanks,

js.

mrpritchett commented 7 years ago

Breaking WP Core functionality is not a feature and this is not a normal priority bug. Can we please see this elevated and a hotfix released?

ChrisWiegman commented 7 years ago

Agreed with both, breaking core functionality that will affect a number of plugins from Bit.ly to YOURLs and more when a non-destructive action is available is not a good way to do this. @jsmoriss have you done a PR for the fix yet?

jsmoriss commented 7 years ago

There's a few possible fixes, depending on what Automattic intended when they decided to break wp_get_shortlink().

If they want to provide wp.me shortlinks for posts/pages, but not custom post types, then they should return false instead of an empty string (as they currently do) in wpme_get_shortlink(). This will allow wp_get_shortlink() to work as intended for custom post types.

Or, they could test for an empty string in wpme_get_shortlink_handler() and return false from there instead.

If they want to provide wp.me links for all post types, then they should remove the post_type_supports( $post->post_type, 'shortlinks' ) check in wpme_get_shortlink().

If they want to provide wp.me links for the rel="shortlink" HTML tag value - but not for custom post types - then they should unhook the 'wp_shortlink_wp_head' action for custom post types. They should still combine that with one of the above changes to make sure wp_get_shortlink() works as intended though. :)

js.

jsmoriss commented 7 years ago

I guess the easiest thing might be to change this:

function wpme_get_shortlink_handler( $shortlink, $id, $context, $allow_slugs ) {
        return wpme_get_shortlink( $id, $context, $allow_slugs );
}

To this:

function wpme_get_shortlink_handler( $shortlink, $id, $context, $allow_slugs ) {
        $shortlink = wpme_get_shortlink( $id, $context, $allow_slugs );
        return $shortlink === '' ? false : $shortlink;
}

That would leave the original behavior of wpme_get_shortlink() intact -- returning and empty string for errors and unsupported conditions.

js.

jsmoriss commented 7 years ago

On a related note, respect could / should be given to values provided by previous callbacks in the filter array, so another option could be:

function wpme_get_shortlink_handler( $shortlink, $id, $context, $allow_slugs ) {
        if ( $shortlink === false ) {
                $shortlink = wpme_get_shortlink( $id, $context, $allow_slugs );
                if ( $shortlink === '' ) {
                        $shortlink = false;
                }
        }
        return $shortlink;
}

js.

jsmoriss commented 7 years ago

@ChrisWiegman Done. ;-)

https://github.com/Automattic/jetpack/pull/8273/

js.

stale[bot] commented 6 years ago

This issue has been marked as stale. This happened because:

No further action is needed. But it's worth checking if this ticket has clear reproduction steps and it is still reproducible. Feel free to close this issue if you think it's not valid anymore — if you do, please add a brief explanation.