WordPress.org

Make WordPress Core

Opened 15 months ago

Closed 8 months ago

Last modified 5 months ago

#36224 closed enhancement (fixed)

WP_Taxonomy class

Reported by: swissspidy Owned by: swissspidy
Milestone: 4.7 Priority: normal
Severity: normal Version:
Component: Taxonomy Keywords: has-patch has-unit-tests 4.7-early commit needs-dev-note
Focuses: Cc:

Description

In a similar vein to #36217, a WP_Taxonomy class would make many things easier, like helping with documentation and preventing accidental errors.

In my proof-of-concept patch, the global $wp_taxonomies would be an array of WP_Taxonomy objects. The class properties wouldn't change for backward compatibility and any class methods would only add some benefit to it.

I also implemented the ArrayAccess interface because the taxonomy args are often passed as arrays (e.g. in the registered_taxonomy filter.

This ticket should:

  • Introduce the WP_Taxonomy class and have base properties and methods for interacting with a post type object.
  • Apply the new class where it can be used within core.
  • Provide tests for any methods introduced.

Attachments (5)

36224.diff (20.3 KB) - added by swissspidy 15 months ago.
36224.2.diff (21.5 KB) - added by swissspidy 15 months ago.
36224.3.diff (22.2 KB) - added by swissspidy 11 months ago.
36224.4.diff (21.9 KB) - added by swissspidy 9 months ago.
36224.5.diff (22.0 KB) - added by swissspidy 8 months ago.

Download all attachments as: .zip

Change History (33)

@swissspidy
15 months ago

#1 follow-up: @danielbachhuber
15 months ago

a WP_Taxonomy class would make many things easier, like helping with documentation and preventing accidental errors.

Can you provide some specific examples?

@swissspidy
15 months ago

#2 in reply to: ↑ 1 @swissspidy
15 months ago

  • Type changed from defect (bug) to enhancement

Replying to danielbachhuber:

a WP_Taxonomy class would make many things easier, like helping with documentation and preventing accidental errors.

Can you provide some specific examples?

  • Improved documentation on DevHub, moving/copying lots of information out of register_taxonomy() into a separate page.
  • Better type hinting thanks to PHPDoc changes.
  • Forward compatibility (think magic getter/setter for deprecated arguments, improving the way we handle default values, etc.)
  • Better error prevention, see comment:7:ticket:35922 for an example.
Last edited 15 months ago by swissspidy (previous) (diff)

#3 @Compute
14 months ago

There's a var_dump($term->taxonomy,$tax->cap,current_user_can( $tax->cap->edit_terms ) ); in the file src/wp-includes/link-template.php

#4 @ocean90
13 months ago

  • Milestone changed from Awaiting Review to 4.6

@DrewAPicture @boonebgorges How do you feel about this?

#5 @flixos90
13 months ago

See #36217: Many of the recent comments in there probably apply to the approach for this class as well.

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


12 months ago

#7 @chriscct7
12 months ago

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

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


12 months ago

#9 @voldemortensen
12 months ago

  • Milestone changed from 4.6 to Future Release

Not much movement lately, punting fro 4.6. Please move back if an updated patch is produced. (I see a var_dump in the patch still).

@swissspidy
11 months ago

#10 @swissspidy
11 months ago

Updated the patch with the learnings from #36217 and added tests for everything.

Happy to re-consider this for 4.6 if I get some more feedback on both of these tickets.

#11 @swissspidy
11 months ago

  • Keywords 4.7-early added

#12 @swissspidy
9 months ago

  • Milestone changed from Future Release to 4.7

#13 @wonderboymusic
9 months ago

  • Keywords needs-refresh added

Patch needs to be rebooted

@swissspidy
9 months ago

#14 @swissspidy
9 months ago

  • Keywords needs-refresh removed

Latest patch applies cleanly against trunk.

Suggestions on the approach are welcome as always.

#15 @swissspidy
9 months ago

Perhaps needs a __toString() method, see #37368.

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


9 months ago

#17 @jorbin
8 months ago

@swissspidy What do you need to get this over the finish line?

#18 @swissspidy
8 months ago

Still missing some important feedback from component maintainers (@wonderboymusic, @boonebgorges) and @DrewAPicture as requested earlier.

From what I've heard, some people are not that happy with the WP_Post_Type class, mainly because of its size. Maybe there are some significant points worth improving in the current patch.

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


8 months ago

#20 follow-up: @boonebgorges
8 months ago

Thanks for working on this, @swissspidy. General approach looks fine to me.

What is the purpose of the /* @var WP $wp */ blocks? For an IDE?

What are the aspects of WP_Post_Type that people don't like? Is there something in the thread at #36217?

In the future, it may make sense to move functionality into the class, especially functionality that's used only when taxonomies are registered. Stuff like get_taxonomy_labels().

Do we want to set a $db property instead of calling $wpdb? See #37699.

#21 in reply to: ↑ 20 @swissspidy
8 months ago

Replying to boonebgorges:

What is the purpose of the /* @var WP $wp */ blocks? For an IDE?

Yeah, it's for the IDE. Rarely used in core, so I can remove it if unwanted.

What are the aspects of WP_Post_Type that people don't like? Is there something in the thread at #36217?

Mostly on twitter. tl;dr: final keyword, no interface, mutable, should use __get() and __set() instead of plenty of properties and set_props().

In the future, it may make sense to move functionality into the class, especially functionality that's used only when taxonomies are registered. Stuff like get_taxonomy_labels().

Agreed. See #37667 and #38218 for post type examples.

Do we want to set a $db property instead of calling $wpdb? See #37699.

Not really a fan of that. See 83:ticket:37699

#22 follow-up: @boonebgorges
8 months ago

Mostly on twitter. tl;dr: final keyword, no interface, mutable, should use get() and set() instead of plenty of properties and set_props().

It seems to me that the goal of this ticket, and the WP_Post_Type ticket, is to bring a small amount of sanity to what is currently a free-for-all stdClass fracas. We are not attempting to impose any actual OOP architecture. We *should* impose some architecture on it in the future, but it doesn't necessarily have to happen all at once, and I don't see that anything in the proposed patch affects our ability to do it down the road. Backward compatibility will probably always require that existing properties be mutable, whether or not they're hidden behind __set(). And final prevents us from having to make any decisions about the purpose and design of the interface for the time being.

@swissspidy
8 months ago

#23 in reply to: ↑ 22 @swissspidy
8 months ago

  • Keywords commit added; dev-feedback removed

Replying to boonebgorges:

Mostly on twitter. tl;dr: final keyword, no interface, mutable, should use get() and set() instead of plenty of properties and set_props().

It seems to me that the goal of this ticket, and the WP_Post_Type ticket, is to bring a small amount of sanity to what is currently a free-for-all stdClass fracas. We are not attempting to impose any actual OOP architecture. We *should* impose some architecture on it in the future, but it doesn't necessarily have to happen all at once, and I don't see that anything in the proposed patch affects our ability to do it down the road. Backward compatibility will probably always require that existing properties be mutable, whether or not they're hidden behind __set(). And final prevents us from having to make any decisions about the purpose and design of the interface for the time being.

100% agreed.

I uploaded 36224.5.diff which makes the patch apply cleanly again. If there are no final objections I'd like to commit this. I can create new tickets for further improvements, just like the post type tickets I mentioned before.

#25 @swissspidy
8 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 38747:

Taxonomy: Introduce WP_Taxonomy and use it in register_taxonomy() and unregister_taxonomy().

This changes the global $wp_taxonomies to an array of WP_Taxonomy objects. WP_Taxonomy includes methods to handle rewrite rules and hooks.
Each taxonomy argument becomes a property of WP_Taxonomy. Introducing such a class makes further improvements in the future much more feasible.

Props boonebgorges for review.
Fixes #36224. See #36217.

#26 @swissspidy
8 months ago

  • Keywords needs-dev-note added

#27 follow-up: @sglifestylee
5 months ago

error

Fatal error: Class 'WP_Taxonomy' not found in /home/u267678536/public_html/wp-includes/taxonomy.php on line 384

what should i do

#28 in reply to: ↑ 27 @johnbillion
5 months ago

Replying to sglifestylee:
It sounds like your site's update to 4.7 is incomplete. Sorry to hear this, it doesn't happen very often. You should try updating WordPress again, or try reinstalling it. If you need any help, you can try the support forums.

Note: See TracTickets for help on using tickets.