WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 8 months ago

#14760 closed enhancement (fixed)

wp_get_shortlink not working on pages

Reported by: mattsay Owned by: ryan
Milestone: 3.7 Priority: normal
Severity: normal Version: 3.0
Component: General Keywords: has-patch needs-testing dev-feedback
Focuses: Cc:

Description

wp_get_shortlink doesn't return anything on pages.

reason:

2196   if ( isset($post->post_type) && 'post' == $post->post_type )
2197         $shortlink = home_url('?p=' . $post->ID);

Attachments (8)

wp_get_shortlink.diff (1.3 KB) - added by sillybean 3 years ago.
Generates p= shortlinks for all content types, not just posts.
wp_get_shortlink-2.diff (1.1 KB) - added by sillybean 3 years ago.
wp_get_shortlink-3.diff (1.1 KB) - added by layotte 3 years ago.
14760.diff (4.2 KB) - added by ryan 8 months ago.
Refresh with unit tests
14760.2.diff (4.1 KB) - added by ryan 8 months ago.
Don't return shortlinks when cruft-free links aren't enabled
14760-button.diff (866 bytes) - added by sillybean 8 months ago.
Hide "Get Shortlink" button if no shorter link is available
14760-button2.diff (890 bytes) - added by sillybean 8 months ago.
14760-button3.diff​ (1.0 KB) - added by sillybean 8 months ago.
Add page_id= check to Get Permalink button conditional

Download all attachments as: .zip

Change History (38)

comment:1 Otto424 years ago

Doesn't work on custom post types either.

comment:2 nacin3 years ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to Future Release
  • Type changed from feature request to enhancement

sillybean3 years ago

Generates p= shortlinks for all content types, not just posts.

comment:3 sillybean3 years ago

  • Cc steph@… added
  • Keywords has-patch needs-testing added; needs-patch removed

comment:4 sillybean3 years ago

  • Version set to 3.0

Updated patch removes some cruft and makes sure shortlinks are generated only for public post types.

layotte3 years ago

comment:5 follow-up: layotte3 years ago

  • Cc lew@… added

Updated patch to remove the permalink_structure check. If the user does not have permalinks turned on, this function returns a blank string as the shortlink. It should be permalink agnostic... so plugins/themes can use it without worrying about permalink checks.

Thoughts?

comment:6 navjotjsingh3 years ago

  • Cc navjotjsingh@… added

comment:7 sillybean3 years ago

The latest patch is great.

comment:8 Otto423 years ago

+1 to latest patch.

comment:9 mikecorkum9 months ago

+1 to path

Version 0, edited 9 months ago by mikecorkum (next)

comment:10 enej9 months ago

+1 to this patch.

comment:11 sillybean8 months ago

  • Keywords dev-feedback added

comment:12 ryan8 months ago

See also #18632

ryan8 months ago

Refresh with unit tests

comment:13 in reply to: ↑ 5 nacin8 months ago

Replying to layotte:

Updated patch to remove the permalink_structure check. If the user does not have permalinks turned on, this function returns a blank string as the shortlink. It should be permalink agnostic... so plugins/themes can use it without worrying about permalink checks.

The reason why it returns a blank when the permalinks are off is because it isn't a valid shortlink — it's the same length as the existing link.

I see the argument that plugins/themes should be able to use it without checking the permalink structure (though really, if it returns nothing, then just call get_permalink() as a fallback — this already must occur since it was so limited to post type) — but the idea was to not show a "Get Shortlink" button if we weren't actually showing something shorter.

comment:14 Otto428 months ago

Yeah, that's a pretty awful return value there. If the normal link is as short as it gets, then it should return the normal link. It's short too. Relying on plugins/themes to check for blank and call get_permalink is a bit silly.

I can see the use case for wanting to not show the button, however in that case then this function is badly named. The name "wp_get_shortlink" implies that it will return a shortlink. It doesn't imply that it will return something different based on external factors. If you want to check for that sort of thing, then that should be in something like "has_shortlink" or something.

ryan8 months ago

Don't return shortlinks when cruft-free links aren't enabled

comment:15 ryan8 months ago

.2.diff is without the change of return value.

comment:16 follow-up: ryan8 months ago

I'm leaning toward changing the behavior. Predictable is good.

comment:17 sillybean8 months ago

Otto beat me to it. Having the function return a blank is certainly not what most developers would expect. If there's no shorter link, it should just return the original link.

comment:18 in reply to: ↑ 16 nacin8 months ago

Replying to ryan:

I'm leaning toward changing the behavior. Predictable is good.

I'm fine with that but the button should be hidden in those cases.

comment:19 sillybean8 months ago

Do you mean the "Get Shortlink" button on the Edit Post screen? In order to hide that when no shorter link is available, wouldn't we have to check for any shortlink-related plugins?

comment:20 Otto428 months ago

A shortlink plugin would hook into this and presumably return a different shortlink, so that works either way.

sillybean8 months ago

Hide "Get Shortlink" button if no shorter link is available

comment:21 sillybean8 months ago

Something like this​, then? (14760-button.diff.)

comment:22 nacin8 months ago

As a plugin could return nothing from that filter (which is a valid return value, and will still be used for non-public post types), the check should probably be $shortlink && $shortlink !== get_permalink( $post->ID ).

sillybean8 months ago

comment:23 sillybean8 months ago

Updated the button patch. (I'm not integrating Ryan's because I'm having trouble checking out the unit tests.)

comment:24 ryan8 months ago

  • Milestone changed from Future Release to 3.7

comment:25 ryan8 months ago

In 25030:

wp_get_shortlink() improvements.

  • Return shortlinks for pages and public CPTs.
  • Return shortlinks even when cruft-free links are not enabled.
  • Unit tests

Props sillybean, layotte, cais
fixes #18632
see #14760

comment:26 follow-up: ryan8 months ago

Since the permalink for page IDs uses ?page_id= instead of ?p=, we'll need to add a case to the buttom patch.

Last edited 8 months ago by ryan (previous) (diff)

comment:27 in reply to: ↑ 26 ryan8 months ago

Replying to ryan:

Since the permalink for page IDs uses ?page_id= instead of ?p=, we'll need to add a case to the buttom patch.

Unless we think that qualifies as a shortlink. Technically, it is shorter, but meh.

comment:28 sillybean8 months ago

Actually, I don't think we need a case. If there's no permalink structure set, we fail the '' != get_option('permalink_structure') test and wp_get_shortlink returns ?page_id=. The button doesn't show. If there is a permalink structure, the button shows. We get a ?p= shortlink instead of ?page_id=, but since it just redirects, I don't see that as a problem.

comment:29 ryan8 months ago

With permalinks turned off, I see the "Get Shortlink" button when editing a page. The permalink is page_id=2 and the shortlink is p=2.

sillybean8 months ago

Add page_id= check to Get Permalink button conditional

comment:30 ryan8 months ago

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

In 25122:

Don't show the "Get Shortlink" button for pages with a ?page_id=x permalink.

Props sillybean
fixes #14760

Note: See TracTickets for help on using tickets.