This pull request changed or added PHP files in previous commits, but none have been detected in the latest commit.

Thank you! ❤️

Size Change: +559 B (0%)

Total Size: 1.7 MB

_{compressed-size-action}

While it's a bit awkward passing values to the query object that won't really be used in the query, but instead will be used as part of the API url, it feels to me like a natural place for the code calling getEntityRecords to place it, and if it's something that's particular to the revisions kind then maybe it'd kinda work? Also I quite like how you're taking the value and then discarding it before passing the rest of the query on via getQueryArgs.

Thanks for looking at this idea 🙇🏻

I think my inexperience with this package shows 😄

But it seems to handle things, I guess we just need to work out a better interface. I think perhaps a wrapper for getEntityRecord(s) might work better?

For better or worse, the following also crossed my mind:

// A wrapper for `getEntityRevisions`
getEntityRevisionsRecords( 'revisions', `${ postType }`, `${ parentId }`, { ...query } );

// An extra options arg to house stuff. 
getEntityRecords( 'revisions', `${ postType }`, { ...query }, {
    parent: parentId,
} );

I've been tinkering with different ideas today. Jotting down some very random findings and questions.

At this stage Gutenberg only displays global styles revisions, but my instinct is to build something that other entity types (post, page, templates) can hook into.

The first question would therefore be: Where to place revisions in state?
Should revisions "belong" to specific entities, e.g., state.entities.records?.[ kind ]?.[ name ]?.revisions.

Loosely, I think it should be filed under entity name (post type), e.g.,

{
    revisions: {
        parentId: {
            items: { revisionId: { ...revisionData } },
            ...
        } 
   }
}

This is what I was trying in https://github.com/WordPress/gutenberg/compare/try/core-data-entity-revisions-under-name

Or could they be a separate top-level item state.revisions?.[ name ]

{
    revisions: {
        [ name ]: {
                parentId: {
                   items: { revisionId: { ...revisionData } },
                    ...
            } 
         }
   }
}

Either way, it would be great to use existing query reducers to track what's already in state. I haven't very far with that yet, but I suppose they'll need to be modified, along with a bunch of other code that expect specific properties.

By query, I mean modifying REST query params such as { page: 2, per_page: 40 } etc

I was thinking we could invalidate the query cache if the revision count has changed.

Furthermore, give the above we could, later, deprecate state.themeGlobalStyleRevisions[ currentGlobalStylesId ].

Maybe I'm overthinking it. It's a lot of work and might introduce bugs at first, so another approach would be to just implement global styles revisions (all new code and top-level state item) and custom query handling in state. state.themeGlobalStyleRevisions[ currentGlobalStylesId ] could still exist until deprecated, but it would return the default set (without query params). But all could introduce more tech debt. 🤷🏻

Or just concentrate on global styles revisions and not worry about caching state according to query (means more API calls) for now.

@youknowriad I know you've got a lot on, but when you get a spare moment it would be great to hear your thoughts. I'm wary of touching core data and breaking stuff, but don't want to repeat too much either. 🙏🏻

This is a very quick though passing around here.

• It seems there's no canonical revision id, so we're forced to pass a couple (id, parent)
• I don't like that we're stretching the definition of "entities" to fetch "revisions". It feels to me like we're doing this just to reuse code but conceptually these are different: A revision is a property of another record, it's not a thing on its own. So IMO, maybe we should avoid using getEntityRecord and getEntityRecords for revisions and instead have a "supports: revisions" in the config of each entity which would automatically allow fetching revisions of that particular entity.

So IMO, maybe we should avoid using getEntityRecord and getEntityRecords for revisions and instead have a "supports: revisions" in the config of each entity which would automatically allow fetching revisions of that particular entity

I appreciate the feedback - it's very valuable, thank you. I'll have another poke around.

It feels to me like we're doing this just to reuse code

The main concern I had was not to reproduce logic surrounding query caching, but maybe it's best to do it longhand and look for consolidation opportunities later. It would at least suit me if I were to carry on: me having not much experience with the entity types and models in the plugin 😄

I think it would be valuable to have other opinions here as well. cc @WordPress/gutenberg-core @jsnajdr

I agree that query caching and handling is a bit of a monster and that reusing it helps a lot. Maybe there are ways to reuse these sub-reducers/sub-selectors without allowing fetching revisions as entity records.

Anyway, I'm very open to more discussion and opinions here :)

To me post revisions seem to be a full-featured REST entity on its own. For a given post type and post ID, there is a collection of revisions, and also individual revisions. Each of them having their own REST resource URL.

The relationship between a post and its revisions is similar to the relationship between a post and its attachments (media) or its comments. They are all linked to in the _links meta-field in the /posts/$id response.

It seems there's no canonical revision id, so we're forced to pass a couple (id, parent)

Yes this is one thing that's unusual about revisions. A revision has a perfectly unique and canonical revision id -- after all, it's just a post like any other, with post type revision. But there's no REST URL where it could be fetched from using just the id. There must be the "parent post type + post id" prefix:

/wp/v2/posts/1/revisions/2

The endpoint doesn't even care if the revision 2's parent is really the post 1. If will happily return the same revision from URL like:

/wp/v2/pages/3/revisions/2

It merely checks that pages/3 exists, that's all.

Could making the revisions(version-history) embeddable in core help us with this? Would that be simpler or there are more nuances? 🤔

If we were to consider revisions an entity.

• What would getEntityRecords( 'post', 'revisions' ) return (query is optional) ? All the revisions of all the posts?
• I'm also guessing we'd have to support tupples as ids: something like getEntityRecord( 'post', 'revisions', { postId: 10, revisionId: 13 } ) or come up with some convention getEntityRecord( 'post', 'revisions', '10|13' )

I think revisions are "sub entities" and not top level entities and in that sense, our framework doesn't support sub entities yet.

I'm really grateful for the discussion. I was swimming in the dark on this one 😄

What would getEntityRecords( 'post', 'revisions' ) return (query is optional) ? All the revisions of all the posts?

I'd guess that it would return a set of revisions according to the default collection params?

So max items 100, ordered by date descending.

I'm also guessing we'd have to support tupples as ids: something like getEntityRecord( 'post', 'revisions', { postId: 10, revisionId: 13 } ) or come up with some convention getEntityRecord( 'post', 'revisions', '10|13' )

I was smitten with an earlier idea of yours: to create a dedicate selector. This could remove the need for argument gymnastics.

getEntityRecordRevisions( 'post', parentId, { ...pageQueries } ) // returns all revisions constrained by any query params.

getEntityRecordRevision( 'post', parentId, [ ...revisionIds ] ) // returns specific revisions.

Could making the revisions(version-history) embeddable in core help us with this? Would that be simpler or there are more nuances?

Would that impact response size for posts with lots of revisions? Or could we embed a subset? 🤔

I think revisions are "sub entities" and not top level entities and in that sense, our framework doesn't support sub entities yet.

Interesting. As in state.entities[subEntity]? Or deeper, beside the same kind/name? E.g., state.entities.records[ kind ][ name ].revisions

Where do folks think it would make sense to build this into the existing state?

At first I was imagining something like this:

{
    entities: {
       // existing entity records
        records: {
            [ postType: kind ]: {
                [ post: name ]: {
                    queriedData: { items, itemIsComplete, queries },
                    edits: { ... },
                    saving: { ... },
                    ..., 
                }
            }
        },
        // sub entity 
        revisions: {
                [ post: name ]: {
                    [ parentId ]: {
                        queriedData: { items, queries, ... },
                    },
                    [ parentId ]: {
                        queriedData: { items, queries, ... },
                    },
                }
       }
    },
}

They are all linked to in the _links meta-field in the /posts/$id response.

I'm guessing we'd fetch the entity record first and then grab the revision URL? E.g, item?._links?.[ 'version-history' ]?.[ 0 ]?.href

I suppose I'm wondering if we need to care about the revisions URL in the same way we do about top-level entity baseURLs?

Where do folks think it would make sense to build this into the existing state?

I think that's not important at all, it's implementation details, so whatever feels easy. For me, the thing to get right is the external API (selectors and actions).

What would getEntityRecords( 'post', 'revisions' ) return

This collection doesn't exist, at least there is no REST API to fetch it.

I'm also guessing we'd have to support tuples as ids

Yes, this is IMO the only thing where post revisions differ from other entities. And the tuple wouldn't be the id (key) but the entity name. Like:

getEntityRecords( 'postType', 'post:1:revisions' ); // collection
getEntityRecord( 'postType', 'post:1:revisions', 2 ); // item

The first two arguments, 'postType' and 'post:...', are kind of forced. Because to derive the REST URL for post revisions, like /wp/v2/posts/1/revisions we need the entity config for the post post type, because we need its rest_base and rest_namespace fields. And if you look at the getEntityRecord resolver:

https://github.com/WordPress/gutenberg/blob/trunk/packages/core-data/src/resolvers.js

it needs kind = 'postType' and name = 'post' to retrieve the config. Then it can work through the rest of the arguments (rest of name, key and query) to construct the revisions URL.

Could making the revisions(version-history) embeddable in core help us with this?

Not really because things that are embeddable are always already independent entities. Embbeding author or replies is just a shortcut that saves one REST request, or getEntityRecord call for something that's already an entity.

But here we don't even have an agreement that revisions are a real entity 🙂

I don't like that we're stretching the definition of "entities" to fetch "revisions". It feels to me like we're doing this just to reuse code but conceptually these are different:

If we started supporting sub-entities, how would the API look like? Some nested call like this?

getSubEntityRecords( getEntity( 'postType', 'post', 1 ), 'revisions' );

But that's not really different at all from a compound entity name:

getEntityRecords( 'postType', 'post:1:revisions' );

A revision is a property of another record, it's not a thing on its own.

I disagree here. Revisions are an entity on its own, simply because they have their own REST endpoint. That's the necessary and sufficient condition.

Comments and attachments are also "properties or another record": the REST response for a post contains links to them in the _links section (links like /media?parent=1 or /comments?post=1). And yet they are at the same time independent entities, there is no conflict is that.

Revisions just have a weird name or key, that's all.

This collection doesn't exist, at least there is no REST API to fetch it.

It doesn't matter if it doesn't exist, the API in core-data exists If { kind: "post", name: "revisions" } is defined as an entity.

getEntityRecords( 'postType', 'post:1:revisions' ); // collection
getEntityRecord( 'postType', 'post:1:revisions', 2 ); // item

While this makes more sense in terms of API calls . I don't think we ever planned for "name" to be this dynamic: aka create an entity per post ID. Also it doesn't make sense because the config for post:1:revisions and post:x:revisions is the same and we'd have to register a config each time we add a post and remove a config each time we remove a post. Entities are meant to be more static.

If we started supporting sub-entities, how would the API look like? Some nested call like this?

I don't think we need to necessarily support entities in a generic way from the start. I think we should start by just supporting "revisions" for entities

getEntityRecordRevision( kind, name, recordId, revisionId )
getEntityRecordRevisions( kind, name, recordId )

Potentially we could support sub entities more generically later, if there's a need to fetch more sub entities outside of revisions. but there's no need for a generic selector.

It doesn't matter if it doesn't exist, the API in core-data exists If { kind: "post", name: "revisions" } is defined as an entity.

If name is a tuple, then revisions name doesn't really exist, there is only post:1:revisions. But I get it, this problem exists, many selectors in core-data are autogenerated, for singular and plural variants. For example, if you call wp.data.select('core').getUnstableBases(), you'll see a really strange result. Only the singular selector makes sense for singleton entities like root/__unstableBase, but the plural selector is generated anyway.

I don't think we ever planned for "name" to be this dynamic: aka create an entity per post ID. Also it doesn't make sense because the config for post:1:revisions and post:x:revisions is the same

We never planned for this, but suprisingly, it could work very well 🙂 The configs for both posts are only almost the same: the baseURL is different because it includes the post ID. You use only the first part of the tuple name to retrieve the kind+name entity config, and then patch is slightly to create a revisions entity.

getEntityRecordRevision( kind, name, recordId, revisionId )

This is the same thing as getEntityRecord( kind, ${name}:${recordId}:revisions, revisionId ). And, what I think is most important for our discussion, I think the actual implementation will be 80% the same for getEntityRecordRevision and getEntityRecord with tuple name. It will have to retrieve the entity for kind+name, construct the base URL, then append /revisions/id to it, store the response in some .revisions subtree of Redux state... I don't see much difference there.

So, I don't mind starting with getEntityRecordRevision. Maybe it will turn out to be easily mergeable into an extended getEntityRecord, maybe not.

Once again, thanks for sharing your knowledge. 🍺

And, what I think is most important for our discussion, I think the actual implementation will be 80% the same for getEntityRecordRevision and getEntityRecord with tuple name. It will have to retrieve the entity for kind+name, construct the base URL, then append /revisions/id to it, store the response in some .revisions subtree of Redux state

This could be a silly question: I was wondering, could revisions be its own "kind"? That way it could run through the post types that support revisions.

I tried that using a tuple-like key:

wp.data.select( 'core' ).getEntityRecords( 'revisions', 'post:10' )

Here is state.entities.records tree:

A far few unnecessary properties that could be pruned from the reducer, e.g., saving/edits/deleting

To do this I created a callback getBaseUrl() in the entity config.

I was wondering, could revisions be its own "kind"?

To answer this we need to know what kind actually means -- why don't we have just entity name? Most of the time kind and name act as a compound key, in code like if (a.kind === b.kind && a.name === b.name ) or state[ kind ][ name ]. Why not merge them together?

The only place where kind is alone is when loading entity configs. root configs are hardcoded, postType configs are loaded from the /wp/v2/types REST endpoint, taxonomy configs from /wp/v2/taxonomies.

When deciding whether postTypeRevisions should be a new kind, we need to first ask: when getEntityRecord or other function needs the entity config for post revisions, where does it get it from? It's going to be derived from the postType config, because there is no other source for it. We'll construct a REST URL for a post, and then append /revisions to it.

The answer will follow from how the code for revisions looks like. Does it call

const config = select.getEntitiesConfig( 'postTypeRevisions' );
const revisionsUrl = urlFromConfig( config );

or

const config = select.getEntitiesConfig( 'postType' );
const revisionsUrl = urlFromConfig( config ) + '/revisions';

?

Both is possible, although I personally would intuitively prefer the second option: load entity configs for the postType kind, construct a URL for postType, and then derive the revisions URL from it. In a getEntityRecordsRevisions function, or in getEntityRecords when the name has the post:1:revisions form.

I personally would intuitively prefer the second option: load entity configs for the postType kind, construct a URL for postType, and then derive the revisions URL from it. In a getEntityRecordsRevisions function, or in getEntityRecords when the name has the post:1:revisions form.

Interesting! Thanks again for helping on the thinking.

I had some time today so I hacked something into this POC PR just to test it out (and learn more about the innards of core-data)

// Get all
wp.data.select( 'core' ).getEntityRecords( 'postType', 'post:7:revisions' )

// Get one
wp.data.select( 'core' ).getEntityRecord( 'postType', 'post:7:revisions', 104 )

Flaky tests detected in 68a1581.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6739002166
📝 Reported issues:

• [Flaky Test] should partially select with shift + click (@webkit) #53524 in /test/e2e/specs/editor/various/multi-block-selection.spec.js
• [Flaky Test] should select with shift + click #48390 in /test/e2e/specs/editor/various/multi-block-selection.spec.js

Add more explanatory documentation and examples to packages/core-data/README.md

I'll do this as a follow up.

@andrewserong Thanks for testing again.

I had the same question. See comment.

TL;DR: it's expected 😄

I've updated the test description to use await wp.data.resolveSelect( 'core' ).getRevisions and await wp.data.resolveSelect( 'core' ).getRevision, which I think should be easier to test with as it'll return a promise that'll resolve with the results.

I had the same question. See #54046 (comment).

My original comment was about the null response you get on the first call when the response is not loaded yet. But here you describe a different situation: when the per_page: 2 response is loaded, and you ask for per_page: 3, you initially get the per_page: 2 response, and only when the REST response with per_page: 3 arrives, the selector starts returning the per_page: 3 data. I could also initially return null -- it's a new query -- but it doesn't. The per_page: 2 and per_page: 3 queries are not considered to be that separate. Learning about this was a TIL moment for me, too 🙂 But it works this way consistently across all core queries.

The difference for per_page is confusing to me too. I don't understand why we'd show page 2 results and then switch to page 3 results. If the returned result is the accumulation of all the pages, sure, they should be considered the same query but if we're actually returning just the page 3 in the end, I think that's a bug. But it's independent from this PR, maybe let's track it for now.

Thanks for the explanation, folks!

@andrewserong sorry, your comment flew over my head 🤦🏻 I get it now 😄

I can write up an issue for this.

The reason why I thought it was all fine is because I've been resolving each call via resolveSelect all this time.

So there are 2 x API calls as I'd expect, and the results also match what I'd expected, that is, the correct revision ids in the collection.

when the per_page: 2 response is loaded, and you ask for per_page: 3, you initially get the per_page: 2 response, and only when the REST response with per_page: 3 arrives

Maybe we need to inform the master selector from the queried data selector somehow that the query key doesn't yet exist. My guess is that it's fetching it from the cache despite that. 🤷🏻

Not sure. I can take a quick looksy.

Thanks again all for helping with this PR. It's going to enable a lot of future work.

🍺 🍺 🍺 🍺 🍺 🍺 🍺 🍺 🍺

Note to self: this feature probably deserves a dev note post on https://make.wordpress.org/

Maybe we need to inform the master selector from the queried data selector somehow that the query key doesn't yet exist. My guess is that it's fetching it from the cache despite that.

Issue:

• Core data: selectors return stale data despite different query parameters #56355

Not sure if this is a viable fix, but tried it out:

• [TRY] Core data: extend stable key to use per_page and page query parts #56354

Dev note

Milestones:

Gutenberg 17.2
WordPress 6.5

Two new selectors have to been introduced to Block Editor's Core Data API to fetch revisions and single revisions for post types that support revisions, for example, posts and pages, templates and global styles.

getRevisions( kind, name, recordKey, query ) – fetches a post’s revisions where recordKey is the id of the parent post.

getRevision(kind, name, recordKey, revisionKey, query) – fetches a single post revision, where revisionKey is the id of the individual revision.

The functions use similar arguments to exisiting core data entity functions such as getEntityRecords with the addition of recordKey (post parent id) and revisionKey (single revision id).

Example usage:

// Returns a collection of revisions.
// `parentGlobalStylesId` is the id (int) of the parent post.
wp.data.select( 'core' ).getRevisions( 'root', 'globalStyles', parentGlobalStylesId, { per_page: -1 } );
 
// Paginated results.
// `parentId` is the id (int) of the parent post.
wp.data.select( 'core' ).getRevisions( 'postType', 'post', parentId, { per_page: 3, page: 2 } )
 
// Get a single revision object.
// `parentId` is the id (int) of the parent post.
// `revisionId` is the id (int) of the individual revision post.
wp.data.select( 'core' ).getRevision( 'postType', 'post', parentId, revisionId );
 
 
// Get a single revision with only the id, parent and date fields.
// `parentId` is the id (int) of the parent post.
// `revisionId` is the id (int) of the individual revision post.
wp.data.select( 'core' ).getRevision( 'postType', 'post', parentId, revisionId, { _fields: 'id,parent,date' } );

getRevisions and getRevision can also be used via useSelect in React components:

import { useSelect } from '@wordpress/data';
import { store as coreStore } from '@wordpress/core-data';

function MyComponent() {
    const pageRevisions = useSelect(
        ( select ) =>
            select( coreStore ).getRevisions( 'postType', 'page', pageId ),
        [ pageId ]
    );
    // Do something with pageRevisions...
}

In the background, these selectors’ corresponding resolvers call revisions REST API endpoints.