WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 2 months ago

Last modified 5 weeks ago

#40413 closed enhancement (fixed)

Handle `register_post_type` support's array

Reported by: MaximeCulea Owned by: desrosj
Milestone: 5.3 Priority: normal
Severity: normal Version: 2.9
Component: Posts, Post Types Keywords: has-patch has-unit-tests commit
Focuses: Cc:
PR Number:

Description

When registering a post_type's support with the add_post_type_support() function, it's possible to pass "extra" args to the support, because of the func_num_args which will check if there is a third arg.

While when using register_post_type(), you can't pass any extra args to a support, because it will be stripped by how WP_Post_Type->add_supports works.

Attachments (5)

ticket-40413.patch (1.1 KB) - added by MaximeCulea 3 years ago.
Patch 40413-1
ticket-40413.2.patch (2.0 KB) - added by seuser 2 years ago.
40413.diff (2.0 KB) - added by desrosj 2 months ago.
40413.2.diff (2.3 KB) - added by desrosj 2 months ago.
Update the docblocks for register_post_type() and add_post_type_supports() to describe passing an array for feature support.
rpt-docblock.png (35.9 KB) - added by pbiron 2 months ago.
what the $supports param of register_post_type() will look like in the code reference with the inline example

Download all attachments as: .zip

Change History (31)

@MaximeCulea
3 years ago

Patch 40413-1

#1 @MaximeCulea
3 years ago

  • Keywords has-patch added

So, as I think this behaviour of giving to a support some more extra features is cool, I submit a patch in this direction.
Now from register_post_type you could register a support with extra args as following :
<?php register_post_type( 'post_type', [ 'supports' => [ 'support' => [ extra_1, extra_2 ] ] ] );

#2 @swissspidy
3 years ago

  • Keywords needs-unit-tests added
  • Type changed from defect (bug) to enhancement
  • Version changed from trunk to 2.9

#3 @seuser
2 years ago

  • Keywords has-unit-tests added; needs-unit-tests removed

First shot at something in the Trac so go easy on me :) I've added a test which fails without your patch and now passes with it.

#4 @MaximeCulea
2 years ago

Hi there !
@swissspidy ready to merge ?
Thx

#5 @swissspidy
2 years ago

  • Milestone changed from Awaiting Review to 5.0
  • Owner set to swissspidy
  • Status changed from new to reviewing

#6 @johnbillion
14 months ago

  • Milestone changed from 5.0 to 5.1

#7 @pento
11 months ago

  • Milestone changed from 5.1 to 5.2

This needs reviewing and a decision.

#8 @desrosj
8 months ago

  • Milestone changed from 5.2 to 5.3

This ticket has not received any attention during the 5.2 cycle. With beta 1 tomorrow, going to punt this to 5.3.

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


3 months ago

#10 @desrosj
3 months ago

  • Owner changed from swissspidy to desrosj

@desrosj
2 months ago

#11 @desrosj
2 months ago

  • Keywords 2nd-opinion added

This looks good! The only thing that I am torn on is the variable names.

add_post_type_support()'s documented parameters are $post_type, $feature, $args, but ticket-40413.2.patch names the variables in a way that feels like they are passed in reverse.

40413.diff switches the names, but I am not sure that it makes things clearer because when $args is not an array, it is passed as $feature. I don't have a strong opinion either way, but any feedback would be appreciated.

Also in 40413.diff:

  • Coding standards fixes in the test changes.
  • Added the @ticket tag to the new test function.

#12 @johnbillion
2 months ago

  • Keywords 2nd-opinion removed

Took a look at this and the naming in 40413.diff is preferable as it more closely matches the terminology used in add_post_type_support().

#13 @desrosj
2 months ago

  • Keywords commit added

#14 @desrosj
2 months ago

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

In 46160:

Posts, Post Types: Allow support arguments to be specified when registering post types.

The add_post_type_support() function accepts an optional third parameter that allows extra arguments to be supplied to configure post type support for a given feature. However, because of how register_post_type() and WP_Post_Type->add_supports() work, it is currently impossible to pass these additional arguments when initially registering a post type with register_post_type().

This change makes it possible to supply additional arguments for a feature using the supports argument of register_post_type().

Props MaximeCulea, seuser, desrosj, johnbillion.
Fixes #40413.

#15 follow-up: @desrosj
2 months ago

  • Keywords needs-dev-note added; commit removed

I think this would be a good call out in a miscellaneous changes dev note. A short paragraph with a code example would be very useful for reference.

#16 in reply to: ↑ 15 @pbiron
2 months ago

Replying to desrosj:

I think this would be a good call out in a miscellaneous changes dev note. A short paragraph with a code example would be very useful for reference.

This is a cool new feature! Thanx.

In addition to the dev note, shouldn't the $supports arg in the DocBlock for register_post_type() be updated to describe the new structure?

#17 follow-up: @desrosj
2 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

shouldn't the $supports arg in the DocBlock for register_post_type() be updated to describe the new structure?

@pbiron I did think about that. I didn't only because the docs explicitly mention Serves as an alias for calling add_post_type_support() directly. But more docs wouldn't hurt.

#18 in reply to: ↑ 17 @pbiron
2 months ago

Replying to desrosj:

shouldn't the $supports arg in the DocBlock for register_post_type() be updated to describe the new structure?

@pbiron I did think about that. I didn't only because the docs explicitly mention Serves as an alias for calling add_post_type_support() directly. But more docs wouldn't hurt.

Until I saw this ticket I didn't know that add_post_type_support() could take a 3rd arg (especially since the DocBlock for it doesn't explicitly mention it, see add_post_type_support())...you have to dig into its source to find the array_slice( func_get_args(), 2 ) to figure it out.

@desrosj
2 months ago

Update the docblocks for register_post_type() and add_post_type_supports() to describe passing an array for feature support.

#19 follow-up: @desrosj
2 months ago

40413.2.diff updates both docblocks to detail the ability to pass an array of information for feature support.

I am not too sure on the example being valid inline like that, though.

#20 @MaximeCulea
2 months ago

Thx for the tests @desrosj!

Maybe I am missleading this .. but the concerning here was not a feature request, but rather an enhancement on how register_post_type() works in comparaison with add_post_type_support().

I mean we could more focus on fixing the issue while it has been waiting for long time ;)

If there is a DocBlock missing about how register_post_type(), this could be a different issue ticket?
For me all was good here with comment:14 ;)

#21 @desrosj
2 months ago

@MaximeCulea Ya, this is definitely an enhancement. Sorry for any confusion. "feature" is the nomenclature used by the add_post_type_support() function. The change in [46160] allows the arguments for support of a feature to be passed as an enhancement and will be included in the next release.

I think it's preferable to update the docs for register_post_type() at the same time. Especially since the inline docs for the supports argument of that function is almost a direct copy of the inline docs from add_post_type_support().

@pbiron
2 months ago

what the $supports param of register_post_type() will look like in the code reference with the inline example

#22 in reply to: ↑ 19 @pbiron
2 months ago

Replying to desrosj:

I am not too sure on the example being valid inline like that, though.

rpt-docblock.png is what the $supports param will look like in the Code Reference (if you mean the example in the DocBlock of add_post_type_support() just let me know, and I'll upload a screenshot of that as well).

Note: that screenshot comes from a a plugin I use to generate code references for plugins...that uses the same tools as .org uses for the WP Code Reference (altho the theme is closer to Twenty Seventeen, so it may look slightly different, but it's close enough).

#23 @desrosj
2 months ago

  • Keywords commit added

Thanks, @pbiron!

My main concern was the parser not being able to handle an inline code example that spanned multiple lines.

#24 @pbiron
2 months ago

@desrosj also note: when I created that screenshot I fixed a slight error in the patch (that I thought I mentioned when I posted the screenshot but now see I didn't).

As it stands, the patch will produce a DocBlock that ends with:

...Default is an array. containing 'title' and 'editor'

and that should be:

...Default is an array containing 'title' and 'editor'.

i.e., no full-stop after the word array.

#25 @desrosj
2 months ago

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

In 46246:

Docs: Improve documentation for the supports argument in register_post_type()/add_post_type_support().

Follow up of [46160].

Fixes #40413.

#26 @desrosj
5 weeks ago

  • Keywords needs-dev-note removed

Mentioned in the Miscellaneous Developer Focused Changes dev note for 5.3: https://make.wordpress.org/core/2019/10/15/miscellaneous-developer-focused-changes-in-5-3/

Note: See TracTickets for help on using tickets.