Make WordPress Core

Opened 8 years ago

Closed 7 years ago

Last modified 4 years ago

#36224 closed enhancement (fixed)

WP_Taxonomy class

Reported by: swissspidy's profile swissspidy Owned by: swissspidy's profile swissspidy
Milestone: 4.7 Priority: normal
Severity: normal Version:
Component: Taxonomy Keywords: has-patch has-unit-tests 4.7-early commit has-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 8 years ago.
36224.2.diff (21.5 KB) - added by swissspidy 8 years ago.
36224.3.diff (22.2 KB) - added by swissspidy 8 years ago.
36224.4.diff (21.9 KB) - added by swissspidy 8 years ago.
36224.5.diff (22.0 KB) - added by swissspidy 7 years ago.

Download all attachments as: .zip

Change History (34)

@swissspidy
8 years ago

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

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

#3 @Compute
8 years 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
8 years ago

  • Milestone changed from Awaiting Review to 4.6

@DrewAPicture @boonebgorges How do you feel about this?

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


8 years ago

#7 @chriscct7
8 years ago

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

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


8 years ago

#9 @voldemortensen
8 years 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
8 years ago

#10 @swissspidy
8 years 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
8 years ago

  • Keywords 4.7-early added

#12 @swissspidy
8 years ago

  • Milestone changed from Future Release to 4.7

#13 @wonderboymusic
8 years ago

  • Keywords needs-refresh added

Patch needs to be rebooted

@swissspidy
8 years ago

#14 @swissspidy
8 years ago

  • Keywords needs-refresh removed

Latest patch applies cleanly against trunk.

Suggestions on the approach are welcome as always.

#15 @swissspidy
8 years ago

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

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


8 years ago

#17 @jorbin
7 years ago

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

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


7 years ago

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

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

  • Keywords needs-dev-note added

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

#29 @desrosj
4 years ago

  • Keywords has-dev-note added; needs-dev-note removed

Linking the dev note that was published here for reference: https://make.wordpress.org/core/2016/10/29/wp_taxonomy-in-4-7/

Note: See TracTickets for help on using tickets.