WordPress.org

Make WordPress Core

Opened 18 months ago

Closed 11 months ago

Last modified 8 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 18 months ago.
36224.2.diff (21.5 KB) - added by swissspidy 18 months ago.
36224.3.diff (22.2 KB) - added by swissspidy 14 months ago.
36224.4.diff (21.9 KB) - added by swissspidy 12 months ago.
36224.5.diff (22.0 KB) - added by swissspidy 11 months ago.

Download all attachments as: .zip

Change History (33)

@swissspidy
18 months ago

#1 follow-up: @danielbachhuber
18 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
18 months ago

#2 in reply to: ↑ 1 @swissspidy
18 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 18 months ago by swissspidy (previous) (diff)

#3 @Compute
18 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
16 months ago

  • Milestone changed from Awaiting Review to 4.6

@DrewAPicture @boonebgorges How do you feel about this?

#5 @flixos90
16 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.


15 months ago

#7 @chriscct7
15 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.


15 months ago

#9 @voldemortensen
15 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
14 months ago

#10 @swissspidy
14 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
14 months ago

  • Keywords 4.7-early added

#12 @swissspidy
12 months ago

  • Milestone changed from Future Release to 4.7

#13 @wonderboymusic
12 months ago

  • Keywords needs-refresh added

Patch needs to be rebooted

@swissspidy
12 months ago

#14 @swissspidy
12 months ago

  • Keywords needs-refresh removed

Latest patch applies cleanly against trunk.

Suggestions on the approach are welcome as always.

#15 @swissspidy
12 months ago

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

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


12 months ago

#17 @jorbin
11 months ago

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

#18 @swissspidy
11 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.


11 months ago

#20 follow-up: @boonebgorges
11 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
11 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
11 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
11 months ago

#23 in reply to: ↑ 22 @swissspidy
11 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
11 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
11 months ago

  • Keywords needs-dev-note added

#27 follow-up: @sglifestylee
8 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
8 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.