WordPress.org

Make WordPress Core

Opened 2 years ago

Last modified 19 months ago

#35567 assigned enhancement

New argument `is_embeddable` for `register_post_type()`

Reported by: pampfelimetten Owned by: bradleyt
Milestone: Future Release Priority: normal
Severity: normal Version: 4.4
Component: Embeds Keywords: good-first-bug has-patch has-unit-tests
Focuses: Cc:

Description

I really love the new oembed functions!
But as much as I love the feature, it would be awesome to have an argument in the register_post_type function to choose, if the posttype should be embedable or not. I have no problem with it to be true by default, but I just would love to deactivate it for certain post types which are not that publicly used.

There are already arguments like "publicly_queryable" or "has_archive', so this doesn't seem off.

Attachments (4)

35567.diff (4.4 KB) - added by bradleyt 2 years ago.
35567.2.diff (5.2 KB) - added by bradleyt 2 years ago.
35567.3.diff (6.8 KB) - added by DrewAPicture 2 years ago.
cleanup + 2 tests
35567.4.diff (9.6 KB) - added by swissspidy 2 years ago.

Download all attachments as: .zip

Change History (22)

#1 @pampfelimetten
2 years ago

  • Summary changed from New argument is_embedable for to New argument is_embedable for register_post_type

#2 @swissspidy
2 years ago

  • Keywords needs-patch good-first-bug added
  • Summary changed from New argument is_embedable for register_post_type to New argument `is_embeddable` for `register_post_type()`

That's an interesting suggestion and I not something that has come up before.

Currently, all publicly viewable posts are embeddable. A new is_embeddable flag could inherit from the public property.

I think a patch would be a great base for further discussion.

#3 @swissspidy
2 years ago

See also #35067.

@bradleyt
2 years ago

#4 follow-up: @bradleyt
2 years ago

  • Keywords needs-testing needs-unit-tests added

I think the attahced diff does what is required.

One thing I was unsure of was if the register_post_type arguments are in any particular order?

I am also uncomfortable adding tests, so maybe someone else could do this.

#5 @swissspidy
2 years ago

  • Milestone Awaiting Review deleted
  • Resolution set to invalid
  • Status changed from new to closed
  • Version changed from 4.4.1 to 4.4

#6 @swissspidy
2 years ago

  • Resolution invalid deleted
  • Status changed from closed to reopened

Oops, closed by accident.

#7 @swissspidy
2 years ago

  • Milestone set to Awaiting Review

#8 follow-up: @DrewAPicture
2 years ago

I wonder if maybe we should handle this not as a new argument but instead as a post type support.

#9 in reply to: ↑ 8 ; follow-up: @swissspidy
2 years ago

  • Keywords needs-testing removed

Replying to DrewAPicture:

I wonder if maybe we should handle this not as a new argument but instead as a post type support.

That might make sense, but all core features are directly associated with a functional area of the edit screen (citing the docs for add_post_type_support() here). For embeds, this wouldn't be the case unless we add a UI of some sorts, e.g. a meta box to conditionally disable embeds on a per-post basis.

#10 in reply to: ↑ 9 @DrewAPicture
2 years ago

Replying to swissspidy:

Replying to DrewAPicture:

I wonder if maybe we should handle this not as a new argument but instead as a post type support.

That might make sense, but all core features are directly associated with a functional area of the edit screen (citing the docs for add_post_type_support() here). For embeds, this wouldn't be the case unless we add a UI of some sorts, e.g. a meta box to conditionally disable embeds on a per-post basis.

Fair enough.

#11 in reply to: ↑ 4 @DrewAPicture
2 years ago

  • Keywords has-patch added; needs-patch removed
  • Owner set to bradleyt
  • Status changed from reopened to assigned

Replying to bradleyt:

I think the attahced diff does what is required.

One thing I was unsure of was if the register_post_type arguments are in any particular order?

I am also uncomfortable adding tests, so maybe someone else could do this.

@bradleyt thanks for the patch! Some notes:

Overall:

  • Needs unit tests (/tests/post/types.php is a good place to start)
  • Any new or changed code should follow core coding standards, specifically in regards to spacing.
  • I'm not completely sold on 'is_embeddable' for an argument name. Maybe 'embeddable' or 'can_embed' for the argument would be better.

wp_oembed_add_discovery_links():

  • $post is not passed to this function as it assumes it's in the loop. In this case we can simply use $post_type = get_post_type(). Either way, you'd want to then check whether a post type was returned before trying to use it, perhaps by combining it with the is_singular() check, e.g. if ( is_singular() && $post_type ) {
  • The DocBlock is missing an @since tag. Use "x.x.x" for now.

get_oembed_response_data():

  • $post is required here, so the get_post() call is moot. Instead, you could do $post_type = get_post_type( $post );, or even just pass get_post_type() directly to is_post_type_embeddable() since it's only used once.
  • The @return tag description should be updated to reflect the new condition for the possibility of returning false.

register_post_type():

  • DocBlock is missing a changelog entry for "x.x.x" version for the new argument
  • See Overall above about argument naming

is_post_type_embeddable():

  • The is_scalar() check is unnecessary since it's already done in get_post_type_object(). Should be a post_type_exists() check instead.
  • The DocBlock is missing an @since tag. Use "x.x.x" for now.

Assigning the ticket to mark the good-first-bug as "claimed".

@bradleyt
2 years ago

#12 @bradleyt
2 years ago

@DrewAPicture Firstly, thank you for your feedback. I have uploaded a new patch which I think covers most of your points.

Couple of things I have not done:

  • I have retained the argument name of is_embeddable, until we have some sort of consensus. How would naming decisions like this usually be resolved?
  • There are still no unit tests. I plan to have a go adding these some time next week, unless someone else jumps in first. (My preference would still be for someone with more experience to add the tests, but I am happy to have a go and see how I get on if needed. )

@DrewAPicture
2 years ago

cleanup + 2 tests

#13 @DrewAPicture
2 years ago

  • Keywords has-unit-tests added; needs-unit-tests removed
  • Milestone changed from Awaiting Review to Future Release

35567.3.diff cleans up formatting a bit and adds a couple of tests. Could probably use even more but this is a start.

@swissspidy
2 years ago

#14 @swissspidy
2 years ago

Thanks a bunch for the patches, guys!

Just thinking out loud, 35567.4.diff is a slightly different approach with a filterable is_post_embeddable() function (final name still to be defined). That way, developers can more easily make a single post non-embeddable.

#15 @DrewAPicture
2 years ago

@swissspidy What do you think about is_object_embeddable() instead? I'd hate to stick ourselves with "posts" vernacular. Right now we only support post object embeds, but who's to say we won't add support for embedding comments, terms, sites, or anything else we can think of in the future? :-)

#16 @bradleyt
2 years ago

Hi guys, really like the idea of adding a is_post_embeddable/ is_object_embeddable filter, and changing the function to is_object_embeddable seems sensible.

I have just noticed that manually going to the embed URL for a post which is set as not embeddable will still return the content. This seems OK to me, but I think it would be better to return the "Oops! That embed can't be found." message instead. If so we probably want to add an is_post_embeddable check to the have_posts() conditional on line 34 of embed-template.php?

#17 @swissspidy
23 months ago

I agree that we should keep support for embeds of other objects in mind. At least we should make it as easy as possible.

is_post_embeddable() has the benefit of allowing a default argument and falling back to the global $post easily. is_object_embeddable() sounds like we could use get_queried_object() there. Would love to see such an approach.

@bradleyt You're right, a 404 should be triggered. Not sure if the embed template is the right place to add that check. It's definitely the easiest, but perhaps we should add this in WP_Query(). Ideas welcome.

#18 @swissspidy
19 months ago

I was thinking of enabling embedding single comments today. Is this something that could be more easily achieved using such functionality?

Note: See TracTickets for help on using tickets.