WordPress.org

Make WordPress Core

Opened 4 years ago

Last modified 21 months ago

#42785 assigned enhancement

Change default of `show_in_rest` in register_post_type and register_taxonomy

Reported by: joehoyle Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: REST API Keywords: has-patch needs-unit-tests needs-refresh
Focuses: Cc:

Description

Right now show_in_rest is defaulted to false in register_post_type and register_taxonomy. To improve usefulness of the REST API I think the time has come to include default publicly readable data into the REST APi.

This helps with https://github.com/WordPress/gutenberg/issues/1342#issuecomment-319920850 for Gutenberg too.

I propose we default show_in_rest to true in the following scenarios:

  • Object types registered with public => true (only).
  • Object types registered with publicly_queryable => true.

I have other future ideas for further cases (including authenticated-only cases) but I think this is a good start.

Attachments (4)

42785.diff (1.0 KB) - added by Rahmohn 4 years ago.
42785.2.diff (2.1 KB) - added by rahmohn 4 years ago.
42785.3.diff (4.1 KB) - added by pento 4 years ago.
42785.4.diff (9.7 KB) - added by pento 4 years ago.

Download all attachments as: .zip

Change History (42)

@Rahmohn
4 years ago

#1 @Rahmohn
4 years ago

Hi @joehoyle.

If what I did in register_post_type is correct so I will do the same in register_taxonomy.

#2 @dd32
4 years ago

  • Milestone changed from 4.9.2 to 4.9.3

Bumping to 4.9.3 due to 4.9.2s release

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


4 years ago

#4 @desrosj
4 years ago

  • Milestone changed from 4.9.3 to 4.9.4

Punting to 4.9.4.

#5 @dd32
4 years ago

  • Milestone changed from 4.9.4 to 4.9.5

Bumping, 4.9.4 has been released.

#6 @audrasjb
4 years ago

  • Keywords has-patch 2nd-opinion added

Hello,

I tested this patch on some new custom post types with the following parameters:

  • parameter public set to true : show_in_rest is set to true by default
  • parameter publicly_queryable set to true : show_in_rest is set to true by default

When parameters are combinated with different values, show_in_rest is always set to true by default since there is a OR condition.
Note: concerning this last point, I'd like to ask for a 2nd-opinion if possible.

@Rahmohn in my opinion you can duplicate your patch to register_taxonomy.

Last edited 4 years ago by audrasjb (previous) (diff)

#7 @audrasjb
4 years ago

  • Keywords reporter-feedback added

#8 @Rahmohn
4 years ago

@audrasjb

I will duplicate to register_taxonomy. Thanks for your feedback.

@rahmohn
4 years ago

#9 follow-up: @youknowriad
4 years ago

I propose we default show_in_rest to true in the following scenarios:

Object types registered with public => true (only).
Object types registered with publicly_queryable => true.

I have other future ideas for further cases (including authenticated-only cases) but I think this is a good start.

Hi!

Thanks for working on this, this will greatly help Gutenberg.

To the point above, I think it's important that we follow-up with another patch to force show_in_rest to true (for public CPTs and taxonomies) for loggedin users (other than subscribers) and not being only a default value, because people will expect to be able to use Gutenberg for all CPTs in the backend no matter the value set for show_in_rest.

#10 in reply to: ↑ 9 @Rahmohn
4 years ago

Replying to youknowriad:

I propose we default show_in_rest to true in the following scenarios:

Object types registered with public => true (only).
Object types registered with publicly_queryable => true.

I have other future ideas for further cases (including authenticated-only cases) but I think this is a good start.

Hi!

Thanks for working on this, this will greatly help Gutenberg.

To the point above, I think it's important that we follow-up with another patch to force show_in_rest to true (for public CPTs and taxonomies) for loggedin users (other than subscribers) and not being only a default value, because people will expect to be able to use Gutenberg for all CPTs in the backend no matter the value set for show_in_rest.

I agree with you.

So show_in_rest will be true in the following scenarios:

  • Object types registered with public => true (only).
  • Object types registered with publicly_queryable => true.
  • In the backend (logged users - other than subscribers) for public CPTs and taxonomies no matter the value set for 'show_in_rest'

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


4 years ago

#12 @audrasjb
4 years ago

  • Keywords 2nd-opinion reporter-feedback removed

Hello,

I tested your last patch and it looks good to me. I think this can land in 4.9.5 "as it".

As a reminder, here is the result of this patch, for both custom post types and taxonomies:

  • parameter public set to true : show_in_rest is set to true by default
  • parameter publicly_queryable set to true : show_in_rest is set to true by default

When parameters are combinated with different values, show_in_rest is always set to true by default since there is a OR condition.

I guess another ticket should follow up to force show_in_rest to true for logged in users to help Gutenberg to work in custom post types.

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


4 years ago

#14 @audrasjb
4 years ago

Hello,

As far as I know, it still needs a dev feedback before landing in the next minor.
4.9.5 beta release is planned for next Tuesday so we'll need a confirmation from component mainteners in the next few days otherwise it will be punted to 4.9.6 (which is not that bad).

Last edited 4 years ago by audrasjb (previous) (diff)

#15 @audrasjb
4 years ago

  • Keywords 2nd-opinion added

#16 @pento
4 years ago

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

@pento
4 years ago

#17 @pento
4 years ago

  • Keywords needs-unit-tests needs-refresh added; 2nd-opinion removed

In 42785.3.diff:

  • Simplify the logic for deciding the post type show_in_rest default.
  • Show all post types for logged in users.
  • Show meta for logged in users.
  • Add tests for post meta showing for logged in users.

TODO:

  • Duplicate the new post type logic in taxonomies.
  • Add many more unit tests.
  • Tweak the meta logic for logged in users, to hide meta with a name beginning with _ that hasn't explicitly set show_in_rest to true.

@pento
4 years ago

#18 @pento
4 years ago

In 42785.4.diff:

  • Re-add the logic to taxonomies.
  • Tweaked the meta show logic.
  • Added more tests.

TODO:

  • There's still a bug in the meta show logic, the test_get_registered_underscore_dont_show_logged_in test is failing.
  • More tests.

I'm about to board a long flight, I'd appreciate if someone could take over this ticket to get it ready before 4.9.5b1.

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


4 years ago

#20 @audrasjb
4 years ago

  • Milestone changed from 4.9.5 to 4.9.6

Bumping to 4.9.6 due to 4.9.5 beta release.

#21 @danielbachhuber
3 years ago

I'm unconvinced that show_in_rest should become opt-out instead of opt-in.

Given custom post types probably have custom editor UI (metaboxes and otherwise), it seems unlikely that Gutenberg will be a drop-in replacement for the existing UI. Also, I'm concerned we'd unintentionally expose data we don't mean to expose.

Lastly, such a significant change certainly shouldn't be in a minor release.

For the purposes of Gutenberg, loading the Classic Editor for post types with show_in_rest=false seems sufficient to me.

#22 follow-up: @youknowriad
3 years ago

For the purposes of Gutenberg, loading the Classic Editor for post types with show_in_rest=false seems sufficient to me.

I strongly disagree with this. The classic editor is there only for backwards compatibility concerns, at some point (probably not soon) its code will be removed from Core, so conceptually, Gutenberg should be able to edit all public post types. Without this we won't be able to edit these post types at all.

Last edited 3 years ago by youknowriad (previous) (diff)

#23 in reply to: ↑ 22 @danielbachhuber
3 years ago

Replying to youknowriad:

For the purposes of Gutenberg, loading the Classic Editor for post types with show_in_rest=false seems sufficient to me.

I strongly disagree with this. The classic editor is there only for backwards compatibility concerns, at some point (probably not soon) its code will be removed from Core, so conceptually, Gutenberg should be able to edit all Post Types with show_ui true. Without this we won't be able to edit these post types at all.

I should've been more clear: for the purposes of shipping Gutenberg in WordPress 5.0.

In the short-term, I think we need to keep the Classic Edit for custom post types with show_in_rest=true. In the long-term, we do need to figure out some solution.

#24 @youknowriad
3 years ago

I should've been more clear: for the purposes of shipping Gutenberg in WordPress 5.0.

👍While this could be fine, my concern is that people new to WordPress would have to deal with two editors while it shouldn't be that hard to update show_in_rest to be opt-out for logged-in users.

Aside, In an API world (which is where we're heading), a show_in_rest config doesn't make sense at all. I understand it's here to mitigate the fact that we can unintentionally expose data but from my point of view, exposing data should be a security/access/role/capability concern and not a global config but that's another question obviously.

#25 @danielbachhuber
3 years ago

Aside, In an API world (which is where we're heading), a show_in_rest config doesn't make sense at all.

We've had to make many, many, many design compromises (cough cough, sticky posts) because of WordPress' historical application architecture. You're working with 15+ years of code, not a shiny brand new project.

#26 @youknowriad
3 years ago

You're working with 15+ years of code, not a shiny brand new project.

I understand that, and these compromises are necessary. But that's not a reason to not change anything. We could plan these more difficult changes iteratively. Deprecate things for several versions... Inform the users to force a property to true even if it's its default value and then after some releases, switch it to default false...

Do you have any link to discussions explaining the need for show_in_rest, I'd love to learn more about those compromises?

#27 @danielbachhuber
3 years ago

Do you have any link to discussions explaining the need for show_in_rest, I'd love to learn more about those compromises?

Sure, here's some of the genesis (with links to corresponding Slack conversations):

The nut of it is that we can't expose information publicly unless we can prove that it can be public. For instance, a registered CPT could be using post_excerpt for some secret notes. Because they're secret, the developer hasn't exposed the field in the theme templates. And there's no way for us to declaratively know whether this custom use of data is safe to reveal unless the developer has explicitly marked show_in_rest=true.

If we want to transition WordPress to an API-driven world, we need a better plan than default show_in_rest=true and hope there's no impact.

#28 @youknowriad
3 years ago

Awesome thanks @danielbachhuber These links are indeed very helpful to understand the whole picture and I agree with the compromises made at that time.

If we want to transition WordPress to an API-driven world, we need a better plan than default show_in_rest=true and hope there's no impact.

I think it's not "if we want" anymore, WordPress is transitioning to an API-driven world with Gutenberg.

So, it's better we start planning for this now. I propose we define for each entity what's public, what's not, what capability is needed to see any field. Once done we can add ways for people to force capabilities/roles by fields if possible (for example we can't force the post title, content to be private in APIs for public CPTs), include deprecation warnings and clarify these changes, leave them for some releases and start updating. I also think this is not specific to the REST API, the REST API is just one API, the same considerations concern any other type of API. Imagine we add GraphQL, how do we define which property needs which capability since we can't use a global show_in_rest, GraphQL requests being per field...

I guess we could discuss these things in the weekly meetings unless you think this is set in stone.

#29 @joehoyle
3 years ago

To justify the creation of this ticket a little more - let me spell out why I think we need to do this.

I think primarily we should be focusing on the semantics of what data is readable, by who, what is public etc. Unfortunately the definitions of public in a post type is pretty fraught with exceptions, there's multiple ways in interpret show_ui, publically_queryable etc - for this reason, in the introduction of the REST API we put up a simple wall "show_in_rest" to just be simple about what data is included in the REST API. The obvious issues with this approach is that we 1: defaulted to true, and 2: divorced it of any association with what the user-access is on the data; it encourages the wrong approach.

Perhaps it made sense at the time, given the REST API was a new thing where we wanted to be conservative - but now it's considered (for Gutenberg at least) the source of the data on the WordPress site. It doesn't make sense to have a technology-enabled flag anymore. There's undoubtedly some work to do to define exactly what post types can be read be unauthenticated users, and logged in ones. However, show_in_rest should not be this, and we should start to phase it's existence out. The REST API is no longer a new technology that is relegated to optional activation per cpt - it's a core part of the technology stack of WordPress, which is demonstrated more than ever in the building of Gutenberg.

With the right access controls, there should be no reason by default not to expose the data over the REST API. If you're a developer that's building a shadow post type, then sure - we have a way to opt out, but that's not the norm. The REST API should not be a myopic view into your whole WordPress data, on a developer-only opt-in basis, it should by default, contain access to as much of it as possible (assuming we of course have sane publicity defaults, which I think we pretty much already to).

That all being said, its time to focus on specifics that show_in_rest = true is adversely cause.

---

Random replies:

The nut of it is that we can't expose information publicly unless we can prove that it can be public. For instance, a registered CPT could be using post_excerpt for some secret notes. Because they're secret, the developer hasn't exposed the field in the theme templates. And there's no way for us to declaratively know whether this custom use of data is safe to reveal unless the developer has explicitly marked show_in_rest=true.

This isn't true - this data could, and I think _would_ be in the RSS Feeds, for one thing. If a post type is registered as public, that's the closest we are ever going to get to say something is public.

To the point above, I think it's important that we follow-up with another patch to force show_in_rest to true (for public CPTs and taxonomies) for loggedin users (other than subscribers) and not being only a default value, because people will expect to be able to use Gutenberg for all CPTs in the backend no matter the value set for show_in_rest.

We should be thinking only in capabilities, not roles - or logged in state. Also, nit-pick "all CPTs in the backend no matter the value set for show_in_rest" this isn't true, users will expect to see any CPTs they know about. Just like the rest of the admin, there's no such thing as "show all CTPs" (menus being one example).

#30 @danielbachhuber
3 years ago

So, it's better we start planning for this now. I propose we define for each entity what's public, what's not, what capability is needed to see any field.

Yes, +1 to this. I think this ticket needs to be a research project first, and implementation details later. Personally, I'd suggest tracking down a couple dozen real implementations of CPTs, and then begin evaluating from there.

There's undoubtedly some work to do to define exactly what post types can be read be unauthenticated users, and logged in ones. However, show_in_rest should not be this, and we should start to phase it's existence out.

Exactly this. However, show_in_rest has historically meant "reveal in REST" (whether or not the data is exposed), so I don't think it makes sense to change this on a whim. Possibly, the intermediate implementation could be editable_in_rest and default to true.

The key distinction (based on my understanding of the ticket description):

  • show_in_rest defaulting to true would mean the existence of the CPT is exposed to the world (and possibly have some unexpected information disclosure issues).
  • editable_in_rest would meant the CPT could be editable in the backend with appropriate authorization, but would expose no existence to the unauthorized world.

With the right access controls, there should be no reason by default not to expose the data over the REST API.

Correct. But these aren't present in 42785.4.diff. That patch, for instance, would give me (a WordPress.com user) access to every CPT registered on every CPT site. Not only that, but it also incorrectly overrides the value passed by the developer.

#31 @joehoyle
3 years ago

Exactly this. However, show_in_rest has historically meant "reveal in REST" (whether or not the data is exposed), so I don't think it makes sense to change this on a whim. Possibly, the intermediate implementation could be editable_in_rest and default to true.

I don't think this is the right approach, and also I don't think anything is changing on a whim. Forget any "REST" based flag, we should be saying what is public, and what isn't, and for not-public things, who has access to read them. We have to deal with the backwards compat of show_in_rest (hence this ticket), but introducing more technology specific flags has to be the wrong direction.

This is precisely what I've proposed in this ticket:

I propose we default show_in_rest to true in the following scenarios:

Object types registered with public => true (only).
Object types registered with publicly_queryable => true.

Because those are already public. I think there are more cases to open up more "private" post types to the REST API, however I'm not trying to boil the ocean with this ticket. Even if we think that public => true is too broad, there has to be a case for publicly_queryable => true, as I can get that data from any WordPress site by adding a query var to the URL.

Correct. But these aren't present in 42785.4.diff. That patch, for instance, would give me (a WordPress.com user) access to every CPT registered on every CPT site. Not only that, but it also incorrectly overrides the value passed by the developer.

Yes, I think 42785.4.diff is a very bad hack and should not be committed - is_user_logged_in means next to nothing.

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


3 years ago

#33 @pento
3 years ago

  • Milestone changed from 4.9.6 to 5.0

Bumping to 5.0.

#34 @jorbin
3 years ago

  • Milestone changed from 5.0 to Future Release

Based on the current thinking in https://github.com/WordPress/gutenberg/issues/2457#issuecomment-384501141 that there will need to be some transition time before this can be done, I'm moving this out of the 5.0 milestone.

#35 @andizer
3 years ago

I was looking at the latest diff file and I have one point of consideration. I discovered the following being used twice, why not wrapping this a function, make it reusable?

<?php
$show_in_rest_default = false;
if ( ! empty( $args['public'] ) || ! empty( $args['publicly_queryable'] ) ) {
    $show_in_rest_default = true;
}

Another thing to consider is assigning the if-statement directly to the variable:

<?php
$show_in_rest_default = ( ! empty( $args['public'] ) || ! empty( $args['publicly_queryable'] ) );

This ticket was mentioned in Slack in #core-restapi by timothybjacobs. View the logs.


3 years ago

This ticket was mentioned in Slack in #core-editor by aduth. View the logs.


3 years ago

#38 @pento
21 months ago

  • Owner pento deleted
Note: See TracTickets for help on using tickets.