-
Notifications
You must be signed in to change notification settings - Fork 442
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
base: develop
Are you sure you want to change the base?
Bugfix/#1679 ensure content nodes are public post types #2355
Conversation
…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
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 /**
* 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
); |
Should this PR do the same for |
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. |
@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 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 Anyway, what I think we need to figure out, is GraphQL Type and Field names for:
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 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) 🤔 |
Perhaps my last thought is too aggressive. Maybe changing the 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 |
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 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 |
@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 If the fields returning 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 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
The Schema should reflect that. The |
…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
Code Climate has analyzed commit 3a4874d and detected 0 issues on this pull request. View more on Code Climate. |
@justlevine I updated the PR to only apply the ex:
but, if the taxonomy The downside to this, now, is that if someone were to filter the post type 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 But now the On the upside, terms like 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:
|
[Written before your last commit/comment. Reviewing that now]
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
I guess I'm wondering is whether the expectation from the This rises from the parallel to There's reason to assume 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 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.
}
}
For this and the above, it definitely feels that that |
(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 🤔
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 |
Agree with the issues you raise, and that this is essentially a naming things issue more than anything else.
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 Separately, A
I dont see a shape-based reason for |
I think that obfuscates things even more than just just calling the all types |
Regarding using
Probably (along with the (Sorry for the premature close. Hit the wrong button 😂) |
Some naming I'm considering 🤔 :
or
or
|
As far as terms/taxonomies go, I'm thinking we have a similar problem here: Just like Post Types, taxonomies can be:
So, possibly we go with:
|
I'm still catching up on this thread 😅 but just wanted to share a few thoughts as I read.
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? The reason WordPress has this to say about
This is backed by the way IMO, if the intent of the Gatsby plugin is to not expose those post types to the public, they should register them as
|
I believe that is a mistype. The intention here is that (obstensibly) when
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. |
Yes, they have a URI. Good idea to check that! Screenshot (logged out just to check lack of auth):
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 |
Edited my previous message to fix an incorrect statement I made. 🤦 |
@justlevine @mindctrl correct, this was a typo. @mindctrl ah yes, I've confirmed that 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 => falseI 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 And you can see this content is left out of the sitemap. And the content is not included in search results: I can, however, access the page by it's permalink (I was wrong about that above!) 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 => trueIf I change my post type registration to 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: And the content is included in sitemaps: And the content of this post type is also included in search results: WordPress is treating posts of 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 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 What are they called? How do we query them? etc.
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 |
@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. |
I think since this release https://github.com/wp-graphql/wp-graphql/releases/tag/v1.6.7 |
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 |
It seems that 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 |
@TylerBarnes ya, some posts / pages are private too. I think what we're trying to ultimately figure out, is how can users interact with:
I think we're all largely on the same page that fully public post_types should be queryable together, and the 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: 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. |
@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
Then, we could have our Post Types implement that interface, but not our term types (even though terms also have an ID)
Even though a Category has an ID field, and could technically implement the DataNode interface, we don't want 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 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? |
yes |
Another late arriver to a great conversation. There are two things that strike me upon reading the points here:
I agree with this. (Although I would probably advocate for an even narrower definition with better alignment. Beyond I will sit with my reading a bit more, though. |
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. |
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. |
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. |
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 theUniformResourceIdentifiable
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
After
After this PR, the
ActionMonitorAction
post type is not returned when querying forcontentNodes
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.