New argument `is_embeddable` for `register_post_type()`

[pampfelimetten]

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.

[swissspidy]

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.

[swissspidy]

See also #35067.

[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.

[swissspidy]

Oops, closed by accident.

[DrewAPicture]

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

[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.

[DrewAPicture]

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.

[DrewAPicture]

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]

@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]

cleanup + 2 tests

[DrewAPicture]

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]

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.

[DrewAPicture]

@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? :-)

[bradleyt]

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?

[swissspidy]

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.

[swissspidy]

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

[desrosj]

The patches here need to be refreshed against current trunk.

I'm also going to remove the ticket owner here. @bradleyt's your work is very much appreciated! It's not fair to assume you are still interested/able to continue working on this one. Removing the owner may help convince another contributor to pick up the torch. If you are still interested in continuing the work here, by all means, please do! You can always be re-added as the owner.

[swissspidy]

#54867 was marked as a duplicate.

Rebased Core Trac Ticket changes to today's WordPress code.

Trac ticket: https://core.trac.wordpress.org/ticket/35567

[Gadelhas]

Hello all!

I have updated @bradleyt's work to today's WordPress.

The name is still pretty much open, but would love some comments on it.

Hey @mukeshpanchal27 , thanks for the review.

Just updated the PR with your comments and also the fix for the failing tests.

[Gadelhas]

Ciao @swissspidy ,

Following the conversation from WordCamp, could you take another look at this ticket?

Thanks!