WordPress.org

Make WordPress Core

Opened 9 months ago

Last modified 2 weeks ago

#38310 assigned enhancement

Improve "wp_update_term" documentation

Reported by: ruud@… Owned by: ruudjoyo
Milestone: Future Release Priority: normal
Severity: normal Version: 4.7
Component: Taxonomy Keywords: good-first-bug needs-patch needs-unit-tests
Focuses: docs Cc:

Description

Hi all,

When using the wp_update_term function and reading through its inline docs, it struck me that some part of the inline docs is kind of vague (highly subjective, I know).
I will add a patch with a proposal for a more specific inline docs.

Thanks,
Ruud

Attachments (3)

38310.patch (731 bytes) - added by ruud@… 9 months ago.
Improved docs proposal
38310.diff (539 bytes) - added by mazzomaz 9 months ago.
38310-2.patch (2.7 KB) - added by ruud@… 9 months ago.

Download all attachments as: .zip

Change History (15)

@ruud@…
9 months ago

Improved docs proposal

#1 @ruud@…
9 months ago

  • Keywords has-patch dev-feedback added

#2 @boonebgorges
9 months ago

  • Focuses docs added
  • Keywords needs-patch good-first-bug added; has-patch dev-feedback removed
  • Milestone changed from Awaiting Review to Future Release

Hi @ruud@joyo - Agreed 100% that this bit of the documentation is unclear. Introduced in [6378].

IMHO, the paragraph you've edited should be removed altogether:

  1. It's not actually accurate that the terms/term_taxonomy table schemas give any information about $args.
  2. $args should be documented as a @param anyway. It already is, though it a lackluster and apparently incorrect way. I don't think these $args have anything to do with get_terms().

I think we need a new patch that removes the paragraph in question, and updates the @param documentation to be correct. You should be able to figure out each argument's purpose by looking at the way the $parsed_args parameter is used throughout the function (and $defaults will show you the default values).

@mazzomaz
9 months ago

#3 @mazzomaz
9 months ago

  • Keywords has-patch added; needs-patch removed

Patch uploaded in 38310.diff

#4 @ruud@…
9 months ago

Hi Boone and Mazzomaz,

@boonebgorges, Thanks for thorough description and the clear instructions on how to move this ticket forward
@mazzomaz, Thanks for the patch, removing the paragraph is for the better.

However, while digging through this lengthy function, and thinking of the double 'warning' in the inline-docs =>

  1. 'Care must be taken to not override important information need to update or update will fail (or perhaps create a new term, neither would be acceptable).'
  2. 'check the term scheme can contain and stay away from the term keys.'

I thought why bother with all these 'warnings' while we can just circumvent (un)intentional by removing the term_id from the $args.

Example: not removing the 'term_id' from $args will result in getting a slug from the wrong Term

$slug = wp_unique_term_slug($slug, (object) $args) => $query = $wpdb->prepare( "SELECT slug FROM $wpdb->terms WHERE slug = %s AND term_id != %d", $slug, $term->term_id );

If the 'term_id' is missing and/or removed the sanitize_term() function will correct this anyway to its default, but after the array_merge( $term, $args ) which I think is important for not overwriting the original term_id.

Disclaimer: I do not know any of the background of this function, or it's use in the broader WP eco-system. So please let me know if this patch doesn't make sense at all.

Thanks,
Ruud

@ruud@…
9 months ago

#5 @ruud@…
9 months ago

  • Keywords dev-feedback needs-testing added

#6 @boonebgorges
9 months ago

Hi @ruud@joyo - From what I can see, passing term_id as part of the $args array will *already* be ignored. The only $args values that do anything are: 'name', 'description', 'parent', 'term_group'.

The "Care must be taken..." line is another one that doesn't make much sense for modern WordPress. We use $args from a whitelist. It's not possible to modify existing terms in way other than these whitelisted arguments. Maybe what's meant is: if you don't want to change the 'description' field, don't pass a 'description' key, etc. If so, this could use some rewording. Originally added in [8164].

#7 follow-up: @ruud@…
9 months ago

Hi @boonebgorges,

Thanks for your reply.

From what I can see, passing term_id as part of the $args array will *already* be ignored. The only $args values that do anything are: 'name', 'description', 'parent', 'term_group'.

I think your mostly right, except that the entire $args array is also passed to wp_unique_term_slug on line 2908:
$slug = wp_unique_term_slug($slug, (object) $args);

None of the other $args values are filtered out and 'survive' the entire function to this point and further along.

The "Care must be taken..." line is another one that doesn't make much sense for modern WordPress.

Indeed.

We use $args from a whitelist. It's not possible to modify existing terms in way other than these whitelisted arguments.

Not entirely true, I think it is possible to alter the slug via a slug from another term (via a different term_id via $args), see remark about the wp_unique_term_slug() function in use.

Maybe what's meant is: if you don't want to change the 'description' field, don't pass a 'description' key, etc. If so, this could use some rewording.

Indeed, I'm not a big fan of mentioning things in the docs which might/could happen if used in some way etc. Stating what a function does do, seems a much more prudent way. Adding some more clarification in the @params could help?

I'm curious if I'm missing the obvious here with the possibility of mis-using a 'term_id' in $args here.

Thanks
Ruud

#8 @DrewAPicture
3 weeks ago

  • Owner set to ruudjoyo
  • Status changed from new to assigned

Assigning ownership to mark the good-first-bug as "claimed".

#9 in reply to: ↑ 7 @DrewAPicture
3 weeks ago

Replying to ruud@…:

Indeed, I'm not a big fan of mentioning things in the docs which might/could happen if used in some way etc. Stating what a function does do, seems a much more prudent way. Adding some more clarification in the @params could help?

The value in occasionally describing what could happen is when one piece of functionality is commonly used with another and discrepancies (that we can't necessarily f ix) cause behavior perceived as unexpected.

#10 @boonebgorges
3 weeks ago

  • Keywords has-patch dev-feedback removed

Hi @ruud@joyo - Apologies for the silence on this ticket.

You're right about the args that are being passed through to wp_unique_term_slug(). The long description in the doc block should probably make this more explicit.

The unset( $args['term_id'] ) change does seem like it might fix a bug, but I think we need more explicit description of the bug before dropping in this change. I think it would be a unit test that: 1. sets up terms A and B, 2. updates term A ($term_id) and but pass $args['term_id'] = B, 3. show how the generated slug is incorrect. It may turn out that $args['term_id'] = $term_id is a more appropriate fix than simply unsetting.

#11 @flixos90
2 weeks ago

  • Keywords needs-patch needs-unit-tests added; needs-testing removed

Adding appropriate keywords (as far as I understand the discussion) - please adjust if I chose the wrong ones. :)

This ticket was mentioned in Slack in #core by welcher. View the logs.


2 weeks ago

Note: See TracTickets for help on using tickets.