WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#30999 closed defect (bug) (fixed)

Inconsistent parameters for edited_term_taxonomy action

Reported by: ipm-frommen Owned by: boonebgorges
Milestone: 4.2 Priority: normal
Severity: normal Version: 3.0
Component: Taxonomy Keywords: dev-feedback needs-unit-tests has-patch
Focuses: Cc:

Description

There are three edited_term_taxonomy action hooks:

The first one provides as second parameter $taxonomy the taxonomy slug, while the others provide the taxonomy object.

Attachments (1)

30999.patch (1.7 KB) - added by ipm-frommen 3 years ago.
Pass taxonomy name instead of object

Download all attachments as: .zip

Change History (15)

#1 @ipm-frommen
3 years ago

  • Keywords needs-patch dev-feedback added

#2 @valendesigns
3 years ago

It appears that edit_term_taxonomy is also inconsistent in what is being passed around.

#3 @valendesigns
3 years ago

  • Keywords needs-unit-tests added
  • Version set to 3.0

The offending code was added in changesets 13625 & 19035. Though it is a valid bug and should be discussed and patched, keep in mind it's been a bug for 5 years and could have backwards compatibility concerns. So I'll assume this ticket is going to need unit tests before a patch will even be considered.

#4 follow-up: @boonebgorges
3 years ago

valendesigns - Thanks for tracking down the changesets.

I'm not currently near my computer that has a checkout of the full plugin repo, but having a look at the repo would be a good next step. If it turns out that, say, the vast majority of plugins are expecting the taxonomy object, it'd make it easier to decide how to solve this problem. (Unit tests wouldn't hurt, but are not critical in this case - the main issue will be to inform devs of the change.)

#5 @toscho
3 years ago

We could limit the damage if we would turn the $taxonomy object into a real data type, i.e. a class WP_Taxonomy, with a __toString() method.

#6 in reply to: ↑ 4 @ipm-frommen
3 years ago

Replying to boonebgorges:

I'm not currently near my computer that has a checkout of the full plugin repo, but having a look at the repo would be a good next step. If it turns out that, say, the vast majority of plugins are expecting the taxonomy object, it'd make it easier to decide how to solve this problem.

Did you have a look at the full plugin repo? What does the vast majority of the plugins hooking to the edited_term_taxonomy action expect?


Replying to toscho:

We could limit the damage if we would turn the $taxonomy object into a real data type, i.e. a class WP_Taxonomy, with a __toString() method.

I really like the idea (not just for taxonomies, BTW). This would, of course, be a substantial change and goes beyond the scope of this ticket. But anyway, we could start by using the new class only in the three functions related to this issue. This would be a minimal change as this, right?

class WP_Taxonomy {

    public function __construct( stdClass $taxonomy ) {

        foreach ( get_object_vars( $taxonomy ) as $key => $value ) {
            $this->{ $key } = $value;
        }
    }

    public function __toString() {

        return $this->name;
    }

}

In the wp_update_term function, all we'd have to do is this:

$taxonomy_object = new WP_Taxonomy( get_taxonomy( $taxonomy ) );
do_action( 'edited_term_taxonomy', $tt_id, $taxonomy_object );

Same for edit_term_taxonomy, as mentioned by valendesigns.

#7 follow-up: @SergeyBiryukov
3 years ago

Searching for edited_term_taxonomy in the plugin directory yields 29 results:

011-ps-custom-taxonomy/ps-custom-taxonomy.php
012-ps-multi-languages/includes/ps-multilingual-taxonomy.php
achievements/includes/achievements/functions.php
anspress/includes/anspress-ranks.php
anspress-question-answer/includes/anspress-ranks.php
awesome-support/includes/functions-custom-fields.php
co-authors-plus/co-authors-plus.php
connections/includes/class.terms.php
content-audit/content-audit-fields.php
easysocial/inc/ranks.php
file-gallery/file-gallery.php
getmecooking-recipe-template/recipe-template-functions.php
intellectual-property-basic/taxonomies/company/setup.php
mark-posts/public/class-mark-posts.php
maven-algolia/admin/controllers/indexer.php
media-features/media-features.php
organize-series/orgSeries-utility.php
pay2post/lib/framework/includes/term-counts.php
piklist/includes/class-piklist-taxonomy.php
polylang/include/model.php
rtbiz/app/lib/rt-attributes/class-rt-attributes.php
rtbiz/app/lib/rt-offerings/class-rt-offerings.php
shopp/core/model/Collection.php
simple-groups/simple-groups.php
smsify/modules/usergroups/UserGroups.php
user-groups/user-groups.php
user-tags/UserTags.php
user-taxonomies/user-taxonomies.php
wp-autoloader/lib/WPExtend/BasicWPActions.php

2 of those plugins treat $taxonomy as a string:

  • Co-Authors Plus
  • GetMeCooking Recipe Template

5 plugins treat $taxonomy as an object:

  • Maven Algolia
  • Organize Series
  • Pay2Post
  • Polylang
  • rtBiz

The rest appear to be unaffected by the change of type.

#8 in reply to: ↑ 7 @ipm-frommen
3 years ago

Replying to SergeyBiryukov:

Searching for edited_term_taxonomy in the plugin directory yields 29 results: [...]

2 of those plugins treat $taxonomy as a string:

  • Co-Authors Plus
  • GetMeCooking Recipe Template

5 plugins treat $taxonomy as an object:

  • Maven Algolia
  • Organize Series
  • Pay2Post
  • Polylang
  • rtBiz

The rest appear to be unaffected by the change of type.

Thanks for checking this out, Sergey.

Could you please do this for edit_term_taxonmy as well?

So far, we have 2:1 for taxonomy objects wrt. action hooks, and 5:2 wrt. plugin expectations.

#9 @boonebgorges
3 years ago

SergeyBiryukov - Thanks so much for looking this up. I did a similar search on 'edit_term_taxonomy' and only found one plugin that uses the $taxonomy parameter: user-series. It expects a string.

My inclination is to settle on passing a string (the taxonomy slug). Elsewhere in taxonomy.php (and WP) we generally pass names to filters, not the corresponding objects.

If others are OK with this, I'll reach out to the authors of the 5 affected plugins with suggestions for a fix.

@ipm-frommen
3 years ago

Pass taxonomy name instead of object

#10 @ipm-frommen
3 years ago

  • Keywords has-patch added; needs-patch removed

#11 @SergeyBiryukov
3 years ago

  • Milestone changed from Awaiting Review to 4.2

#12 @boonebgorges
3 years ago

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

In 31525:

Pass taxonomy name, not object, to 'edit_term_taxonomy' and 'edited_term_taxonomy' actions.

These actions are fired in a number of different places, and in some cases
the tax name is passed, while in others the taxonomy object is passed. This
inconsistency made it difficult for plugins to use the $taxonomy value.

Props ipm-frommen.
Fixes #30999.

#13 follow-up: @boonebgorges
3 years ago

I'll be reaching out individually to the plugin authors listed above to warn them about the change in [31525]. I'll also put a note on make/core once #18828 has been settled one way or another.

#14 in reply to: ↑ 13 @Chouby
3 years ago

Replying to boonebgorges:

I'll be reaching out individually to the plugin authors listed above to warn them about the change in [31525].

Thanks a lot for this. Polylang will be ready for WP 4.2 :)

Note: See TracTickets for help on using tickets.