Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bugfix/#1679 ensure content nodes are public post types #2355

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from

Conversation

jasonbahl
Copy link
Collaborator

@jasonbahl jasonbahl commented Apr 28, 2022

What does this implement/fix? Explain your changes.

This updates the ContentNode type to represent public post types, instead of all post types.

The ContentNode Interface was intended to be shared by public post types. The ContentNode Interface implements the UniformResourceIdentifiable Interface, which is an Interface for public facing nodes that can be identified by a URI.

Non Public post types should not be considered Content Nodes as they don't have a public URI.

The following are examples of Post Types in WordPress core that are not public, don't have a URI, and should not be considered a ContentNode in WPGraphQL:

Does this close any currently open issues?

Any other comments?

Before

Querying a list of content nodes exposed "utility" post type content, such as the ActionMonitorAction type in the WPGatsby plugin.

The action monitor type is intended to be a Publicly Queryable type, but is not publicly identifiable by URI, and therefore should not be considered a ContentNode

CleanShot 2022-04-28 at 12 08 42

After

After this PR, the ActionMonitorAction post type is not returned when querying for contentNodes

CleanShot 2022-04-28 at 12 10 35

BREAKING CHANGE?

This can be a breaking change for some users. It's not a technical breaking change to the Schema, but it could impact users that were querying non-public post types and getting that data back in the context of ContentNode queries. So, while it doesn't.

It's possible, via filters, to add the functionality back (allow other Types to be treated as Content Nodes) should that be very important for specific users.

…wed_post_types properties

- add new _clear_allowed_taxonomies() and _clear_allowed_post_types() methods for help with testing
…s, $allowed_post_types properties"

This reverts commit 2960e54.
…maps input args to the post_types that should be resolved for `ContentNode` queries

- update 'ContentNode' connections to only use public post types
- Update description for the `ContentNode` Type
- Update tests to reflect changes to contenNode connections not returning _all_ post types, but just public post types
@jasonbahl jasonbahl added Compat: Breaking Change This is a breaking change to existing functionality ObjectType: Post Issues related to the Post object type in WP Component: Interfaces Component: Query labels Apr 28, 2022
@coveralls
Copy link

coveralls commented Apr 28, 2022

Coverage Status

Coverage increased (+0.007%) to 79.281% when pulling 3a4874d on jasonbahl:bugfix/#1679-ensure-content-nodes-are-public-post-types into f600479 on wp-graphql:develop.

@justlevine
Copy link
Collaborator

Filter for reverting.

Needs testing that it catches all use cases, since we're we cant directly overwrite the constructor param passed to the resolver, only overwrite the post_type query arg that the PostObjectConnectionResolver set. (Are there other where args besides contentTypes that influence which post_type is set?)

/**
 * Include non-public post types in `contentNode
 **/
add_filter(
  'graphql_post_object_connection_query_args', 
  function( array $query_args, $source, array $args, $context, $info) {
    if( 'contentNodes' === $info->fieldName ) && empty( $args['where']['contentTypes'] ) {
      $query_args['post_type'] = \WPGraphQL::get_allowed_post_types();
    }

    return $query_args;
  }, 
  10,
  5
);

@justlevine
Copy link
Collaborator

justlevine commented Apr 29, 2022

Should this PR do the same for termNodes? Currently they return terms from all taxonomies, and not just from public terms.

@jasonbahl
Copy link
Collaborator Author

Should this PR do the same for termNodes? Currently they return terms from all taxonomies, and not just from public terms.

Yes, I think now would be a good time to address both. Good call out. I'll follow up, and also add some tests for filtering back to old behavior, that way we can document to folks how they can get the old behavior if they feel like it was a feature, not a bug.

@jasonbahl
Copy link
Collaborator Author

@justlevine I think we'll need to come up with a name for these other types of Post Types and Taxonomies. . .like core WP has private taxonomies such as:

For menus (and menu_items), we leave them out and provide a custom way to interact with menus in the Schema, independent from the mapping of post types/taxonomies to the Schema.

For these other ones, right now WPGraphQL doesn't set these to show_in_graphql => true, but of course users could filter them to show_in_graphql => true, and we need ensure they're treated properly in that case.

I do think that if they are private, they shouldn't be exposed in most of the connections we have today, but I do think we should consider how they should be exposed, as the data is likely important, even if only to authenticated parties.

I'd like to find a name that is declarative of what these things are. In my mind I consider them Utility Post Types and Utility Taxonomies but I don't think those, (specifically "Post Types") translate well to a GraphQL Schema. Taxonomies and Terms are more widely used words in technology, but "Post Types" don't really describe what these things are, unless you're a WordPress developer, but a lot of folks interacting with a GraphQL Schema are not WordPress developers and I've tried (very imperfectly) to reduce the amount of WordPress-isms in the Schema. (i.e. Post Type => ContentNode, post_tag => Tag, etc).

Anyway, what I think we need to figure out, is GraphQL Type and Field names for:

  • Public Post Types: ContentType / ContentNode

  • Private Post Types: ? / ?

  • Public Taxonomies: Taxonomy / TermNode

  • Private Taxonomies: PrivateTaxonomy? UtilityTaxonomy? / PrivateTermNode? UtilityTermNode?


In my experience, Taxonomies and Terms are used almost the same whether they're private or public, the difference usually being public ones have archive pages and private ones don't, but they're used to group things together.

Post Types have wider range of utility, in my experience.

Public Post Types are typically for public facing content, but private post types are all over the place. Contact Forms, Redirects, Reusable Gutenberg Blocks, CSS for Customizer, Changesets for Customizer, Revisions (of any other post type), Oembed Cache, User Request (for GDPR, I believe?), etc.

The use of Post Types is soooo wide.


Perhaps, we simply don't expose private post types and taxonomies at all, even if they are set to show_in_graphql and even if there are use cases where admin users would want to query the data?

The assumption being that a post type or taxonomy marked private has a "different" use than standard public post types, and they should be mapped to the Schema in a more specific way (like we do with Nav Menus / Nav Menu Items) 🤔

@jasonbahl
Copy link
Collaborator Author

Perhaps my last thought is too aggressive. Maybe changing the ContentNode and TermNode interface, for now, is good enough 🤔

Then, the "utility" post types and taxonomies can register their own connections, etc as needed.

We can still expose them in the Schema if they are show_in_graphql => true but just leave them out of the "public facing" ContentNode / TermNode interfaces? 🤔

@justlevine
Copy link
Collaborator

justlevine commented Apr 29, 2022

We can still expose them in the Schema if they are show_in_graphql => true but just leave them out of the "public facing" ContentNode / TermNode interfaces? 🤔

I was just thinking this too.

Alternative approach: what if we included everything that is publicly queryable (both post types and taxonomies) but just set the default where args to public? It bypasses the issue, while still providing flexibility and honoring the WP API meaning of publicly_queryable. Instead, we just rely on the URI interface for public types.

I'm still playing catch-up with GraphQL in non WordPress contexts, but so far my understanding is interfaces are best used to describe the shape of a type, not the context it's used in. A ContentNode isn't just a Node thats public, it's a generic name for types that are shaped like WP_Post_Type.

@jasonbahl
Copy link
Collaborator Author

I'm still playing catch-up with GraphQL in non WordPress contexts, but so far my understanding is interfaces are best used to describe the shape of a type, not the context it's used in. A ContentNode isn't just a Node thats public, it's a generic name for types that are shaped like WP_Post_Type

@justlevine most of the utility post types I described store data the way post types store data, but are used and shaped very differently.

For example most of them don't have a uri, which is a field declared by the ContentNode interface. And many are intended to be used in vastly different ways (like redirects, oembed cache, etc). Their shape and utility are very different from other Post Types that are used for "content"/

Also, the Schema will show all Types that implement that interface in the docs, which is a mismatch of behavior.

Seeing that you could query { contentNodes { nodes { ...someFields } } } and the Schema showing that you might get a CssChangeset or an OembedCache is misleading to the client developer working with the Schema.

If the fields returning ContentNode are returning only public post types, then the user looking at the Schema should be presented with an Interface that only Public Types implement.

I think there's benefit to having another Interface (not named ContentNode) that would spread across all PostTypes, public or private. I don't know what to name this thing, and I don't like PostType or PostNode as a name. 😆

I do think there are use cases where folks want to be able to query across literally any post type, much like you might do:

new WP_Query( [ 
  'post_type' => 'any' 
]);

But querying for contentNodes the expectation is that you're doing:

new WP_Query( [ 
  'post_type' => 'public_post_types' 
]);

The Schema should reflect that. The ContentNode interface should only represent Public post types that users can expect as possible return Types of a contentNodes query

…s to use in TermNode connections

- Update `ContentTypeEnum` to use updated WPGraphQL::get_allowed_post_types()` method and `WPGraphQL::get_allowed_taxonomies()` methods
- Update description of `TermNode` interface
- Update Term types in the Schema to only implement the `TermNode` Interface if they're of a public Taxonomy
…p-graphql#1679-ensure-content-nodes-are-public-post-types

# Conflicts:
#	src/Connection/PostObjects.php
#	src/Connection/TermObjects.php
#	src/Data/Loader/UserLoader.php
#	src/Type/Enum/ContentTypeEnum.php
#	src/Type/ObjectType/TermObject.php
@codeclimate
Copy link

codeclimate bot commented Apr 29, 2022

Code Climate has analyzed commit 3a4874d and detected 0 issues on this pull request.

View more on Code Climate.

@jasonbahl
Copy link
Collaborator Author

jasonbahl commented Apr 29, 2022

@justlevine I updated the PR to only apply the TermNode Interface to public taxonomy types.

ex:

type Tag implements TermNode

but, if the taxonomy wp_template_part_area (see: https://github.com/WordPress/WordPress/blob/74dcee85d065fe9f700f3459c44b845aba98f01a/wp-includes/taxonomy.php#L208) were filtered to show_in_graphql it would not implement the TermNode interface.

The downside to this, now, is that if someone were to filter the post type wp_template_part and wp_template_part_area to both show_in_graphql there would be no clear way to query the terms associated with the template part 🤔

For example:

add_filter( 'register_post_type_args', function( $args, $post_type ) {

	if ( 'wp_template_part' === $post_type ) {
		$args['show_in_graphql'] = true;
		$args['graphql_single_name'] = 'TemplatePart';
		$args['graphql_plural_name'] = 'TemplateParts';
	}

	return $args;

}, 10, 2 );

add_filter( 'register_taxonomy_args', function( $args, $taxonomy ) {

	if ( 'wp_template_part_area' === $taxonomy ) {
		$args['show_in_graphql'] = true;
		$args['graphql_single_name'] = 'TemplatePartArea';
		$args['graphql_plural_name'] = 'TemplatePartAreas';
	}

	return $args;

}, 10, 2 );

This adds the private post type template_part and private taxonomy template_part_area to the Schema.

But now the TemplatePartArea doesn't implement the TermNode Interface, so this Type is missing fields like description that are added by that interface.

On the upside, terms like Category have connections to public things like enqueuedStylesheets, but the private taxonomy TemplatePartArea does not have connections to enqueuedStylesheets (because that connection is a TermNode->EnqueuedStylesheet connection and now a private Taxonomy doesn't implement the TermNode interface)

I'm confusing myself right now and going to take a break from this and come back to it after the weekend.

Ultimately, I think we need to resolve what I was saying above:


What I think we need to figure out, is GraphQL Type and Field names for:

  • Public Post Types: ContentType / ContentNode

  • Private Post Types: ? / ?

  • All Post Types: ? / ?

  • Public Taxonomies: PublicTaxonomy? / PublicTermNode?

  • Private Taxonomies: PrivateTaxonomy? UtilityTaxonomy? / PrivateTermNode? UtilityTermNode?

  • All Taxonomies: Taxonomy / TermNode

@justlevine
Copy link
Collaborator

[Written before your last commit/comment. Reviewing that now]
@jasonbahl thanks for the clarification

Seeing that you could query { contentNodes { nodes { ...someFields } } } and the Schema showing that you might get a CssChangeset or an OembedCache is misleading to the client developer working with the Schema.

Those types aren't publicly queryable either. We can both agree that at a bare miniminum the client developer is only going to see items that are supposed to be viewable

If the fields returning ContentNode are returning only public post types, then the user looking at the Schema should be presented with an Interface that only Public Types implement.

I think there's benefit to having another Interface (not named ContentNode) that would spread across all PostTypes, public or private. I don't know what to name this thing, and I don't like PostType or PostNode as a name. 😆

I guess I'm wondering is whether the expectation from the ContentNode interface is/should be "Public post type objects", and not as a GraphQL equivalent to new WP_Query( $args_from_where_args ), (still only including items where is_post_type_viewable() is true).

This rises from the parallel to RootQueryToTermNodeConnection which is "clearly" (I say from a users perspective) intended as a counterpart to WP_Term_Query.

There's reason to assume ContentNode is the same as TermNode but with a less confusing name than PostObjectNode. This is strengthened by the fact that ContentType is the equivalent of WP_Post_Type.

If that's not/shouldnt be the expectation, then yeah 100% a new interface/ rootQuery connection is necessary to handle all post type objects types like ActionMonitorAction that are defined to be publicly queryable, but arent strictly "Content". However, that means we'd need to rename ContentType.

E.g. (names for illustration only. We both know how bad I am at naming things 😎):

 query RootQuery {
   taxonomies { # all publicly_queryable taxonomies. Works like get_taxonomies().
     __typename # Taxonomy
   }
  terms{ # the terms for all publicly_queryable taxonomies. Works like new WP_Term_Query().
     __typename # Term
   }
   postTypes{ # all publicly_queryable post types. Works like get_post_types()
     __typename # PostType
   }
   postObjectNodes { # the post objects for all publicly_queryable post types. Works like new WP_Query()
     __typename # PostObjectNode
   }
   contentNodes { # the post objects for all public post types. Essentially a filtered connection to postObjectNodes
     __typename # Maybe it needs a new interface, maybe its just a PostObjectNode.
   }
 }

In my experience, Taxonomies and Terms are used almost the same whether they're private or public, the difference usually being public ones have archive pages and private ones don't, but they're used to group things together.

For this and the above, it definitely feels that that RootQueryToTermNodeConnection definitely should not only be limited public terms, but if we want to set good dx for most use cases, then we can set the default included taxonomies to only those that are public..

@jasonbahl
Copy link
Collaborator Author

jasonbahl commented Apr 29, 2022

(Brain dumping from my phone, so formatting is going to be weird)

Here's an idea:

public post type: ContentType / ContentNode

private post type: PrivateModel / PrivateModelType

all post types: Model / ModelType

🤔

interface Model implements Node {
  id: ID!
}
interface ContentNode implements Model, UniformReaourceIdentifiable { 
  id: ID!
  uri: String
}
type Post implements ContentNode { 
  ... 
}
type Redirect implements PrivateModel { 
  ... 
}

Then a query for contentNodes returns content of public post types, and the schema self documents public post types as the possible types of the ContentNode interface.

A query for "models" would return nodes of any post type

A query for privateModels would return nodes of private post types

@justlevine
Copy link
Collaborator

justlevine commented Apr 30, 2022

Agree with the issues you raise, and that this is essentially a naming things issue more than anything else.

What I think we need to figure out, is GraphQL Type and Field names for:

Public Post Types: ContentType / ContentNode

Private Post Types: ? / ?

All Post Types: ? / ?

Public Taxonomies: PublicTaxonomy? / PublicTermNode?

Private Taxonomies: PrivateTaxonomy? UtilityTaxonomy? / PrivateTermNode? UtilityTermNode?

All Taxonomies: Taxonomy / TermNode

I'm still not 💯convinced that that the interfaces should be based on an attribute value instead of the possible existence an attribute / set of attributes.

In that pattern, we still have Taxonomy / TermNode, but public and private (and for good measure graphql equivalents to show_ui , exclude_from_search etc) terms are retrieved with where args on the generic connection. Remember we can still register specific connections e.g. from RootQuery to publicTaxonomies or publicTermNodes .

Separately, A NodeWithArchive interface could be created that represents 'public' taxonomies (in this context) i.e. those that have an archivePath and maybe even a connection to the object at the end of that path (would be useful for avoiding all the headaches with trying to show archives on the frontend by hardcoding routes / using nodeByUri e.g. #2191 , #2190, #1910 ).

ContentNode still exists, but its treated like NodeWithAuthor or UniformResourceIdentifiable, i.e. as a WP_Post that has Content related fields. Currently ,all PostObject types are a ContentNode and a NodeWithTemplate, but that just isnt true to real-life, or rather it isn't true if you're defining a ContentNode as a public post type.

I dont see a shape-based reason for ContentType to exist separately from whatever the schema's equivalent to WP_Post_Type is called.

@justlevine
Copy link
Collaborator

justlevine commented Apr 30, 2022

Here's an idea:

public post type: ContentType / ContentNode

private post type: PrivateModel / PrivateModelType

all post types: Model / ModelType

🤔

I think that obfuscates things even more than just just calling the all types PostObjectType and PostObjectNode, even with the existence of the Post object type. Why would Model be associated specifically with post objects, instead of all types that that can be modeled (taxonomies, terms, themes, etc)?
We then also need a clear way to differentiate between the abstract php Model and the Post extends Model that is reflected in the schema as just Model.

@justlevine
Copy link
Collaborator

justlevine commented Apr 30, 2022

Regarding using ContentType or ContentNode as either public-facing post types or post types with a specific subset of fields, it seems WordPress core disagrees

There are many different types of content in WordPress. These content types are normally described as Post Types, which may be a little confusing since it refers to all different types of content in WordPress. For example, a post is a specific Post Type, and so is a page.

source

Probably (along with the PostObject usage of ContentNode as an interface) where some of my confusion stems from.
Not saying we need to take our cue from that, but is does beg the question what other developer users will assume when seeing it in a (self-documenting) schema.

(Sorry for the premature close. Hit the wrong button 😂)

@jasonbahl
Copy link
Collaborator Author

Some naming I'm considering 🤔 :

DataNode = object stored in *_posts table
DataType = WP_Post_Object (get_post_type_object())

or

EntryNode = object stored in *_posts table
EntryType = WP_Post_Object (get_post_type_object())

or

Model = object stored in *_posts table
ModelType = WP_Post_Object (get_post_type_object())

@jasonbahl
Copy link
Collaborator Author

As far as terms/taxonomies go, I'm thinking we have a similar problem here:

Just like Post Types, taxonomies can be:

  • 'public' => true/false
  • 'publicly_queryable' => true/false

So, possibly we go with:

TermNode = terms of _any_ taxonomy (get_term() / WP_Term)
ContentTermNode = terms of _public_ taxonomy (get_term() / WP_Term)

Taxonomy = any taxonomy (get_taxonomy() / WP_Taxonomy)
TaxonomyEnum = names of any taxonomy
ContentTaxonomy = any public taxonomy (get_taxonomy / WP_Taxonomy)
ContentTaxonomyEnum = names of any public taxonomy

@mindctrl
Copy link

mindctrl commented May 4, 2022

I'm still catching up on this thread 😅 but just wanted to share a few thoughts as I read.

We can see that we're even getting a URI for the action monitor nodes, but, we really shouldn't because WordPress itself doesn't respect URI for private post types.

Action monitor post types are queryable on the front end. There's an archive page and single post views work too. Maybe you need to flush permalinks?
image

The reason action_monitor is viewable is because publicly_queryable is set to true. Reference.

WordPress has this to say about publicly_queryable:

 *     @type bool         $publicly_queryable    Whether queries can be performed on the front end for the post type
 *                                               as part of parse_request(). Endpoints would include:
 *                                               * ?post_type={post_type_key}
 *                                               * ?{post_type_key}={single_post_slug}
 *                                               * ?{post_type_query_var}={single_post_slug}
 *                                               If not set, the default is inherited from $public.

This is backed by the way is_post_publicly_viewable() and is_post_type_viewable() work.

IMO, if the intent of the Gatsby plugin is to not expose those post types to the public, they should register them as publicly_queryable => false, though I know that's not really the focus of this conversation. Just wanted to mention this as a clarifying point to the whole "public" vs "private" thing. I think it matters because you were saying "But the contentNodes query would still be the common entry point for querying across public types (with a uri).", and in the example of action_monitor types they do have a URI.

Then, you can register a post_type as private => false, but also set it to be publicly_queryable => true which means you can have content in a private type but the content is actually public

register_post_type() doesn't support a private argument as far as I see.

@justlevine
Copy link
Collaborator

@mindctrl

Then, you can register a post_type as private => false

I believe that is a mistype. The intention here is that (obstensibly) when public is true there is a public facing uri, and when it's false there isn't . A "private" post type / taxonomy (i.e. 'public' === false ) can still be publicly_queryable (determines the model permissions for unverified users), and a CPT/tax that is neither public nor publicly queryable can still be queried in WPGraphQL by users with the correct permissions if show_in_graphql is set.

There's an archive page and single post views work too.

Can you confirm whether they have an actual URI or are just accessible with query vars (like in your screenshot). In general I worry that there's too many ways the registration args are used in the wild for us to be able to accurately give various interfaces self-documenting names that aren't tied to WP terminology, but I was under the impression that WPGatsby's action monitor does meet those assumptions.

@mindctrl
Copy link

mindctrl commented May 4, 2022

@justlevine

Can you confirm whether they have an actual URI or are just accessible with query vars (like in your screenshot).

Yes, they have a URI. Good idea to check that! Screenshot (logged out just to check lack of auth):

image

The intention here is that (obstensibly) when public is true there is a public facing uri, and when it's false there isn't .

It's possible for this to be not true, assuming we're talking about the same thing when we say "public" (confusing all around!). e.g. registering post types this way makes posts "public" with a public facing URI:
register_post_type( 'moo', [ 'publicly_queryable' => true, 'public' => false ] );

Edit: ^ my statement wasn't exactly true. It's queryable and accessible, just not at a direct URI. Apologies for the confusion.

In general I worry that there's too many ways the registration args are used in the wild for us to be able to accurately give various interfaces self-documenting names that aren't tied to WP terminology

I think you might be right. In addition to the all the ways people do it, I think there's a very good chance people misunderstand how public and publicly_queryable work (maybe including me 😆).

@mindctrl
Copy link

mindctrl commented May 4, 2022

Edited my previous message to fix an incorrect statement I made. 🤦

@jasonbahl
Copy link
Collaborator Author

jasonbahl commented May 4, 2022

register_post_type() doesn't support a private argument as far as I see.

@justlevine @mindctrl correct, this was a typo. public => true/false is what I meant.


@mindctrl ah yes, I've confirmed that public => false, publicly_queryable => true does indeed generate a proper permalink! And the content is accessible at that permalink 🤔

I must not have been flushing my permalinks during my other tests here.

But! It still leaves these posts out of most other public interfaces, such as sitemaps, search, etc.

So, the idea is that if you know how to get to the thing, you can get to it, publicly, but it's not going to be grouped together with other content that's primarily intended to make up the pages of the site.

public => false

I registered the following post type (and flushed my permalinks):

register_post_type( 'publicly_queryable', [
	'public' => false,
	'label' => 'Publicly Queryable',
	'show_ui' => true,
	'publicly_queryable' => true,
	'show_in_graphql' => true,
	'show_in_rest' => true,
	'graphql_single_name' => 'PubliclyQueryable',
	'graphql_plural_name' => 'PubliclyQueryables'
]);

With public => false the WordPress editor doesn't even give me the "Permalink" setting to view the post.

CleanShot 2022-05-04 at 09 22 09

And you can see this content is left out of the sitemap.

CleanShot 2022-05-04 at 09 16 02

And the content is not included in search results:

CleanShot 2022-05-04 at 09 32 41

I can, however, access the page by it's permalink (I was wrong about that above!)

CleanShot 2022-05-04 at 09 25 05

But, it seems that WordPress is trying hard to prevent this thing from being grouped together with other post types intended to be used as "content".

Hence my theory that there's a difference between "ContentNodes" (public post types) and "Other, not public, post types"

public => true

If I change my post type registration to public => true (and flush my permalinks)

register_post_type( 'publicly_queryable', [
	'public' => true,
	'label' => 'Publicly Queryable',
	'show_ui' => true,
	'publicly_queryable' => true,
	'show_in_graphql' => true,
	'show_in_rest' => true,
	'graphql_single_name' => 'PubliclyQueryable',
	'graphql_plural_name' => 'PubliclyQueryables'
]);

Now the editor gives me a Permalink UI to view the content:

CleanShot 2022-05-04 at 09 45 59

And the content is included in sitemaps:

CleanShot 2022-05-04 at 09 47 23

And the content of this post type is also included in search results:

CleanShot 2022-05-04 at 09 52 00

WordPress is treating posts of public => true post types as "content" that makes up the site, where public => false post types are treated much differently. You can technically access the data of a public => false, publicly_queryable => true post_type, if you knew how (perhaps via a GraphQL query for that specific type), but in general, the data of those post types aren't treated as "content"

And thus, here we are.

How do we define / name these differences in a way that makes sense.

What are the use cases folks need.

When I originally implemented the ContentNode Interface and RootQuery->ContentNode connection, the goal was to query across nodes that are intended to be used as content (public => true) post types.

This PR is aiming to get that working as originally intended.

But, comes at a loss of "how can we interact with these other nodes that are not considered ContentNode"?

What are they called? How do we query them? etc.

I think you might be right. In addition to the all the ways people do it, I think there's a very good chance people misunderstand how public and publicly_queryable work (maybe including me 😆).

Me too! lol. It's evolved over so many years by different people with different context. Even core seems to treat things slightly differently based on how you access data.

For example, the content in the "custom_css" post type is clearly publicly accessible content, as we can see the output of it as a public user, but the post_type is set as private, and not set to publicly_queryable. We should assume that this is fully private, but yet it's output as publicly visible data. So, WordPress registers it as private, but outputs the content as public 🤦🏻‍♂️ . Confusing indeed.

A lot of this is because WordPress was (and still is) built with the assumption that the CMS is also the rendering layer, and with headless we've come to muck things up.

So we (WPGraphQL specifically) rely on these registry rules perhaps more than core WordPress itself, because in monolith WordPress, even if something is public => false, publicly_queryable => false, you can still write a WP_Query or direct sql statement to "break the rules" and get the data anyway, which is the case with the custom_css. It's registered as non-public, non-publicly_queryable content, but core WordPress "breaks the rules" and gets the content and outputs it to the public anyway.

@TylerBarnes
Copy link
Collaborator

IMO, if the intent of the Gatsby plugin is to not expose those post types to the public, they should register them as publicly_queryable => false, though I know that's not really the focus of this conversation. Just wanted to mention this as a clarifying point to the whole "public" vs "private" thing. I think it matters because you were saying "But the contentNodes query would still be the common entry point for querying across public types (with a uri).", and in the example of action_monitor types they do have a URI.

@mindctrl that's required by WPGraphQL as of a recent version for a post type to be public in the api. I made that change because action monitor posts should be publicly queryable by Gatsby sites without auth.

@TylerBarnes
Copy link
Collaborator

I think since this release https://github.com/wp-graphql/wp-graphql/releases/tag/v1.6.7

@TylerBarnes
Copy link
Collaborator

TylerBarnes commented May 4, 2022

It is perhaps a relevant case to this discussion because action monitor posts should be publicly queryable, they don't need a uri, and they're not content. I guess they would fit into the DataNode interface, but also they're public. Some individual action monitor posts are private (anything related to authenticated previews).

@justlevine
Copy link
Collaborator

It seems that $args['rewrite'] is responsible for whether a publicly_queryable type gets a permalink.

Assuming we accept the definition of an interface as a unique grouping of shared fields (https://www.apollographql.com/docs/apollo-server/schema/unions-interfaces/#interface-type), maybe it makes sense to revisit this as part of #1738? What we call the interface is only important after we have a clear definition of what fields the interface groups, and under what conditions they're applied to a given GraphQL type ...

In the interim, we can temp-fix #1679 by adding a isPublic: bool|undefined where arg to contentNodes, and either leave it undefined for a non-breaking workaround, or setting the default value to true for a breaking change that mirror's the original (and possibly future) intent of the connection.

@jasonbahl
Copy link
Collaborator Author

Some individual action monitor posts are private (anything related to authenticated previews).

@TylerBarnes ya, some posts / pages are private too.

I think what we're trying to ultimately figure out, is how can users interact with:

  • Public data that's intended to be represented by a URI, and is stored by WordPress in the *_posts table
  • Public data that should be queryable without authentication, but isn't intended to be included in sitemaps, and the primary use case isn't visiting the thing at a uri (action montitor actions, custom_css, etc)
  • Private data that should only be visible to authenticated users with proper capabilities (contact form submissions, the core "user_request" post_type, etc).

I think we're all largely on the same page that fully public post_types should be queryable together, and the ContentNode interface and { contentNodes { ... } } query solves for that.

For these other things, what do we do with them? How do we expose them to the Schema? What do we call them? What Interfaces are needed to express how they're similar but different than other things? etc?

It's clear that some naming in WordPress is already super confusing. ex: public / publicly_queryable. . .some of us have a decade+ of WordPress experience and are still confused by this.

How can we express in the WPGraphQL Schema what these things are by naming them well and providing good descriptions in the Schema for them? lol.

@jasonbahl
Copy link
Collaborator Author

Assuming we accept the definition of an interface as a unique grouping of shared fields (https://www.apollographql.com/docs/apollo-server/schema/unions-interfaces/#interface-type)

@justlevine ya, so Interfaces ensure Types that implement it all have those fields, but not all Types that have those fields need to implement said interface.

For example, if we had an interface such as DataNode to represent posts of any post type, and we decided the only field that we felt was common across all post types, was an ID, then we could have an interface like so:

interface DataNode implements Node {
  id: ID!
}

Then, we could have our Post Types implement that interface, but not our term types (even though terms also have an ID)

type Post implements DataNode  & Node{
  id: ID!
  title: String
  ...otherFields
}

type Category implements TermNode & Node {
  id: ID!
  name: String
  ...otherFields
}

Even though a Category has an ID field, and could technically implement the DataNode interface, we don't want Category to be referenced as a possibleType of the DataNode interface as we know that querying for a DataNode will never return a Category.

We know that querying for a DataNode, anywhere in the graph, should be limited only to posts (of any post type).

Much like we know that a ContentNode, anywhere in the graph, should be limited to only posts of public post types.

I do agree that there's much overlap with #1738! And, because I'm an over-analyzer (as you all know from this discussion) that PR has been hanging for quite a while. Perhaps now is the time to get that one finalized as well, as they're solving very similar problems.

@justlevine
Copy link
Collaborator

I think we're all largely on the same page that fully public post_types should be queryable together, and the ContentNode interface and { contentNodes { ... } } query solves for that.

🤐

I do agree that there's much overlap with #1738! And, because I'm an over-analyzer (as you all know from this discussion) that PR has been hanging for quite a while. Perhaps now is the time to get that one finalized as well, as they're solving very similar problems.

Agenda item for the upcoming meeting perhaps?

@jasonbahl
Copy link
Collaborator Author

Agenda item for the upcoming meeting perhaps?

yes

@chriszarate
Copy link
Collaborator

Another late arriver to a great conversation. There are two things that strike me upon reading the points here:

  1. There are a lot of dichotomies proposed here—Public vs. Private, Content vs. Data, Function vs. Utility—but all of them turn out to be incredibly subjective. What value do these classifications bring, especially if they will be confusing or controversial?

  2. On top of this, we've established that the implementation details of how WordPress (or a third-party plugin) chooses to register a post type can be confusing even to experienced WordPress devs. Those details (e.g., "rewrite" => true) often have little bearing on how we think about the underlying data—or on how a WPGraphQL consumer might want to query for it. Why automatically perpetuate decisions that a headless implementor might want to revisit?

I think we're all largely on the same page that fully public post_types should be queryable together, and the ContentNode interface and { contentNodes { ... } } query solves for that.

I agree with this. (Although I would probably advocate for an even narrower definition with better alignment. ContentNodes should have post_content. However, I am happy to abandon this if only for backwards compatibility.)

Beyond ContentNode, I think interfaces should be purely aligned with the data model and I would advocate against further "convenience" interfaces that will probably end up subverting how the implementor thinks about their data. If we make it easy for implementors to specify the post types they want to query against a generic interface, I think that's enough.

I will sit with my reading a bit more, though.

@stale
Copy link

stale bot commented Aug 2, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Aug 2, 2022
@stale
Copy link

stale bot commented Sep 1, 2022

This issue has been automatically closed because it has not had recent activity. If you believe this issue is still valid, please open a new issue and mark this as a related issue.

@stale stale bot closed this Sep 1, 2022
@justlevine justlevine reopened this Sep 2, 2022
@stale stale bot removed the stale label Sep 2, 2022
@jasonbahl jasonbahl mentioned this pull request Nov 23, 2022
17 tasks
@stale
Copy link

stale bot commented Dec 1, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the Stale? May need to be revalidated due to prolonged inactivity label Dec 1, 2022
@bhardie bhardie added the not stale Short-circuits stalebot. USE SPARINGLY label Dec 13, 2022
@justlevine justlevine removed the not stale Short-circuits stalebot. USE SPARINGLY label Mar 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Compat: Breaking Change This is a breaking change to existing functionality Component: Interfaces Component: Query ObjectType: Post Issues related to the Post object type in WP Stale? May need to be revalidated due to prolonged inactivity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RootQuery to ContentNode connection returns posts it shouldn't No fields returned on ContentNode query
7 participants