WordPress.org

Make WordPress Core

Opened 8 years ago

Last modified 2 years ago

#10786 assigned defect (bug)

Implementation of %my_taxonomy% in permastructs is incomplete

Reported by: johnbillion Owned by:
Milestone: Future Release Priority: normal
Severity: minor Version: 2.9
Component: Permalinks Keywords: has-patch dev-feedback needs-refresh
Focuses: Cc:

Description

The register_taxonomy() function includes a call to add_rewrite_tag() which should allow for a site's permastruct to include a %my_taxonomy% tag just like you can include a %category% tag or a %tag% tag, however this implementation is incomplete and doesn't work.

Example:

register_taxonomy('genre','post');

should allow you to create a permastruct (from the Settings->Permalinks screen) which includes %genre% in it.

The problem is that get_permalink() doesn't check for custom taxonomies and the replacement of %genre% with your post's genre in the permalink doesn't happen.

Patch upcoming.

Attachments (2)

10786.patch (1.4 KB) - added by johnbillion 8 years ago.
10786.diff (1.7 KB) - added by dd32 8 years ago.

Download all attachments as: .zip

Change History (21)

#1 @ryan
8 years ago

See also #4875 and #9466.

@johnbillion
8 years ago

#2 @johnbillion
8 years ago

  • Keywords has-patch added

Patch.

The patch basically copies the code which handles %category% in permastructs and applies it to custom taxonomies. It loops over each custom post taxonomy and if the taxonomy slug is present in the permstruct, it sets the search and replacement terms.

The patch also supports a default taxonomy term just like %category% supports a default category. This will allow a site (or a plugin) that is using a custom taxonomy to specify the default taxonomy that should be used in the event a post doesn't have a taxonomy term attached to it for the taxonomy used in the permastruct.

#3 @dd32
8 years ago

Thanks for that john, I needed the exact same thing today, so have based a new patch off yours, The main changes are:

  • Use more of the Taxonomy API
  • Better error handling
  • supports custom post_types (Well, It should at least)

@dd32
8 years ago

#4 @dd32
8 years ago

  • Milestone changed from Unassigned to 2.9

My main reason for rewriting it was that it didnt support taxonomies which have multiple object types, nor did it handle the error conditions well (not checking for errorful returns)

I'd suggest its possible to convert the category code over too perhaps..

#5 @johnbillion
8 years ago

Nice patch dd32. +1

#6 @sudar
8 years ago

  • Cc sudar@… added

#7 follow-up: @dd32
8 years ago

  • Keywords commit dev-feedback added; permalinks taxonomy removed

Any issues? Any developer feedback?

There is an issue with this however, If the post/object does not have any attached terms in the tax, and there is no default term, the resulting link is /blahblahblahblah/ (ie. its a blank string between those double slashes)

#8 in reply to: ↑ 7 @johnbillion
8 years ago

Replying to dd32:

If the post/object does not have any attached terms in the tax, and there is no default term, the resulting link is /blahblahblahblah/ (ie. its a blank string between those double slashes)

The only way around this is to force taxonomies to include a default term, which works fine for categories but is undesirable for tag-like taxonomies (you wouldn't want a default tag on your posts for example).

Not sure what to suggest here. One option would be to whack a dash in there if there is no term for that object (producing /blahblah/-/blahblah/) and then omit the taxonomy part from the query when fetching the object from the permalink? It's not ideal but this is a relative edge case anyway and would probably be fine for those who are choosing to use custom taxonomies in their permastruct.

#9 @dd32
8 years ago

Not sure what to suggest here. One option would be to whack a dash in there if there is no term for that object (producing /blahblah/-/blahblah/)

that looks like a good suggestion to me. Any devs want to comment? Or should it be added to Thursdays dev chat agenda?

#10 @filosofo
8 years ago

Is there some way we can do this without importing the chief annoyance of %category%, namely that the category chosen is always just the one with the smallest ID? The fact that you can use the category query tag in a post's permalink has always been sort of a hack. It's a hack because it works sensibly only when a site is using one category per post. No other potential permalink query tag can have multiple values per post.

And can we avoid adding the overhead of looping through all taxonomies per given post type, then retrieving all terms of that taxonomy for that post?

Suggestions:

  • Filter $rewritecode and $rewritereplace so plugins that want to employ outre query tags in permalinks can do so easily.
  • Add a property to a taxonomy that specifies that a post type can be assigned only one term of that taxonomy.
  • Add an argument to add_rewrite_tag() which puts that tag in the $rewritecode pool.

#11 follow-up: @dd32
8 years ago

Is there some way we can do this without importing the chief annoyance of %category%, namely that the category chosen is always just the one with the smallest ID?

Only by adding a filter that says "This is the preferred term for this taxonomy"

And can we avoid adding the overhead of looping through all taxonomies per given post type,

No. You need to loop through the taxonomy list to determine which items exist within the current structure, We could cache that value however, But seems pointless caching it when realistically, given that looping over half a dozen tax's (under normal use) doesnt add much overhead..

then retrieving all terms of that taxonomy for that post?

Caching data again..

  • Filter $rewritecode and $rewritereplace so plugins that want to employ outre query tags in permalinks can do so easily.

WP_Query already supports it, This is purely get_permalink() permalink creation which doesnt have it.

  • Add a property to a taxonomy that specifies that a post type can be assigned only one term of that taxonomy.

Seems pointless, Better to just fall back onto a filter for preffered ordering of the terms when used within a permalink IMO

Add an argument to add_rewrite_tag() which puts that tag in the $rewritecode pool.

The fact that a Rewrite arg is added says "I WANT to use this in rewrites"..

In my opinion, You're looking at optimizing at the wrong layer. If you want to prevent database lookups, an object cache should be used, There needs to be qeries made to load required data once a page load, unless the user choses to cache it. However the actual permalink itself could be cached, which goes onto the "canonical permalinks" stuff. The ideal way to prevent all the lookups would be:

get_permalink():
  if ( has_post_meta('permalink') )
    return post_meta('permalink', 'single'=true);
  else
   Generate permalink
   update_post_meta
   return $link;

on save_post / update_terms_for_post_$id / anything else
  delete_post_meta('permalink')
  OR
  delete_post_meta('permalink') && get_permalink($id);

But thats for a different ticket maybe?

#12 @dd32
8 years ago

any core dev feedback? It'd be nice to get this fixed.. It really limits permalinks for projects which rely on taxonomies in the URL..

#13 in reply to: ↑ 11 ; follow-up: @filosofo
8 years ago

Replying to dd32:

Is there some way we can do this without importing the chief annoyance of %category%, namely that the category chosen is always just the one with the smallest ID?

Only by adding a filter that says "This is the preferred term for this taxonomy"

That filter or some other way of registering a preferred term needs to be in place before we do anything like this. Arbitrarily picking a category sort of made sense even as a hack, because there are people who build their site structure with categories as hierarchical slots holding individual posts. But it doesn't make sense to take that hack for one particular use of one taxonomy and extend it to all taxonomies abstractly.

And can we avoid adding the overhead of looping through all taxonomies per given post type,

No. You need to loop through the taxonomy list to determine which items exist within the current structure, We could cache that value however, But seems pointless caching it when realistically, given that looping over half a dozen tax's (under normal use) doesnt add much overhead..

We can avoid the overhead if we register which taxonomies should be considered in permalink generation. In 2.9 we're now registering which post types should be public; we may as well do it for taxonomies.

then retrieving all terms of that taxonomy for that post?

Caching data again..

Perhaps, but that's a bandaid for a cut that doesn't need to be inflicted in the first place. Why not just require taxonomies to register if they want to be used in permalinks?

  • Filter $rewritecode and $rewritereplace so plugins that want to employ outre query tags in permalinks can do so easily.

WP_Query already supports it, This is purely get_permalink() permalink creation which doesnt have it.

My suggestion seems to have been what you said a from a few months ago, that is basically allow plugins to do it if they want:

Changed 5 months ago by ryan ¶

How about we remove support for any taxonomy other than category from the permalink structure? We shouldn't encourage such things because of performance reasons and because of the lack of a default. %tag% is already broken, so nothing lost.

Changed 5 months ago by DD32 ¶


Hm... Sounds good to me actually. It'll be far better off in the long run.

Should be able to be added by a plugin if anyone actually really wants it too.




Add an argument to add_rewrite_tag() which puts that tag in the $rewritecode pool.

The fact that a Rewrite arg is added says "I WANT to use this in rewrites"..

Yes, but rewrites != permalink structure. A lot of stuff can be used in rewrites that doesn't make sense in a permalink structure, because permalinks are meant to map one-to-one with a post.

#14 @ryan
8 years ago

  • Milestone changed from 2.9 to Future Release

#15 @scribu
8 years ago

  • Keywords commit removed

#16 in reply to: ↑ 13 @aaroncampbell
6 years ago

Replying to filosofo:

Replying to dd32:

Is there some way we can do this without importing the chief annoyance of %category%, namely that the category chosen is always just the one with the smallest ID?

Only by adding a filter that says "This is the preferred term for this taxonomy"

That filter or some other way of registering a preferred term needs to be in place before we do anything like this. Arbitrarily picking a category sort of made sense even as a hack, because there are people who build their site structure with categories as hierarchical slots holding individual posts. But it doesn't make sense to take that hack for one particular use of one taxonomy and extend it to all taxonomies abstractly.

See #18752 - I added a filter to allow a plugin to specify which category to use in the permalink structure instead of forcing it to use the cat with the lowest ID.

Other than that, I kind of like the blahblah/-/blahblah idea for dealing with the cases where there's no term.

#17 @stephenh1988
6 years ago

  • Cc stephen@… added

Will this patch be extended to custom post types too? From what I can see this will only effect post permalinks?

#18 @ryan
4 years ago

  • Owner ryan deleted
  • Status changed from new to assigned

#19 @chriscct7
2 years ago

  • Keywords needs-refresh added
Note: See TracTickets for help on using tickets.