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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Recursively step through edits to track individually changed post meta #10827

Merged
merged 6 commits into from Nov 19, 2018

Conversation

Projects
None yet
6 participants
@earnjam
Contributor

earnjam commented Oct 20, 2018

Description

First off, this may be a terrible idea. 馃槃

In #6505 we noticed that all post meta gets sent any time there is change to any post meta. This has the effect of saving the default value for each registered meta when only one of them is changed.

The reason for this is two part:

  1. All meta shows up in the REST API. Which is good, we need it there to know what is available.
  2. Since all the registered meta values are present in the REST API, they are all initialized in state when Gutenberg is loaded.
  3. When any meta is changed, the full object of all registered meta is being sent as the state
    parameter to the reducer.
  4. In reducer for the EDIT_POST action, it doesn't dive into individual attribute objects, but rather does a full compare of each top-level item passed in edits vs state and replaces the entire object if they are different. Since the full object of registered meta is sent as the state, the full object would be replaced if there was a change to any one meta value. This also means that when edits are tracked, it treats the full meta object as edited, even when only one meta value is actually changed.
  5. Since the full object of all meta is treated as edited, all registered meta is passed to the UPDATE_POST action and sent to the REST API, forcing all registered meta values to be saved in the DB.

My solution here is to pass only the changed meta values as state when edited in a block, then in the reducer, recursively step down into any edited objects.

This has the effect of tracking changes to individual meta values in edit, but also means that only changed meta values are sent via the REST API when a post is saved.

I considered just making an exception for meta in the edits reducer, but I thought it would be better to keep it generic, so that any attribute that used object stores could be granularly updated and actual edits more accurately tracked.

How has this been tested?

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
@danielbachhuber

This comment has been minimized.

Member

danielbachhuber commented Oct 21, 2018

I think I ran into this with #10204 (comment) too.

@earnjam earnjam requested review from aduth and youknowriad Oct 22, 2018

@earnjam

This comment has been minimized.

Contributor

earnjam commented Oct 22, 2018

Want to get some much more knowledgeable eyes on this, since it will involve changes to the way edits are handled. I'm curious about broader impacts it might have.

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Oct 23, 2018

Instead of always considering objects passed to EDIT_POST action as nested edits which can be ambiguous: How do you recreate the whole post object if you don't know if the edits made are "partial" or not.

Exemple: A plugin can call editPost({ customProperty: newObject }) or editPost({ meta: newObject }).

If our intention in the first call is to replace the customProperty value entirely, how do we know it? how does a potential selector called getEditedPost would build the post object if we don't know if it's a partial change in customProperty or a full replacement?


Maybe the solution is to reshape the reducer of edits this way: edits: { [propertyEditedPath]: value } example. (It could be a map or EquivalentKeyMap to be able to use arrays as keys)

edits: {
     ['meta', 'metaKey' ]: "value",
     ["customProperty"]: { foo: "bar" }
}

which also mean we need a corresponding editPostProperty( propertyPath, value ) action.

cc @aduth as you're the most familiar with how we deal with edits.

@aduth

This comment has been minimized.

Member

aduth commented Oct 23, 2018

It's an interesting one 馃檪

Should it really matter if we send redundant data? I don't really understand why the REST API should return an error response for noop (re #10204 (comment)).

It does have an interesting impact on #7409 / #10844, where we'd roughly use a simple Object.keys( edits ) > 0 as a condition of dirtiness. In this case, we'd need to ensure that only the minimal set of meta keys is present and that the top-level key is absent when there are no sub-keys (not considered here yet).

To the idea of reshaping edits: It might serve this case better, but I expect would also have a negative impact on performance of hot-path selectors like getEditedPostAttribute, since we'd need to iterate through the entries just to do a simple lookup.

Looking at the current implementation, it's not clear to me why we should need to be doing anything at all in BlockListBlock. I think ideally editPost receives only the patch of property updates, and the merging occurs in the reducer. This would look more-or-less like what's proposed here, though might need some additional consideration of equality, and omitting empty parent objects.

@aduth

This comment has been minimized.

Member

aduth commented Oct 23, 2018

REST API design question: Is it safe to assume that any values of type object we send with a PUT request would be treated as a patch update (as it's occurring here with meta)?

@aduth aduth self-assigned this Oct 23, 2018

@aduth

This comment has been minimized.

Member

aduth commented Oct 23, 2018

For your consideration, I've pushed a couple commits to the branch, the result of explorations considered in #10827 (comment) .

Namely:

  • Edits for object properties are treated as patch application, recursively
    • e.g. editPost( { meta: { a: 1 } } ) then editPost( { meta: { b: 2 } } ) yields { edits: { meta: { a: 1, b: 2 } } }
  • Remove all meta merging from BlockListBlock, pass as patch object
  • Improve diff / merge to consider nested properties (omit top-level key if same)

This should both goals of #6505 and #7409.

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Oct 23, 2018

@aduth How do you address the ambiguity raised here #10827 (comment) Where you don't really know what to return from . getEditedPostAttribute because the knowledge of whether it's partial or full editor is know in the server only?

@aduth

This comment has been minimized.

Member

aduth commented Oct 23, 2018

because the knowledge of whether it's partial or full editor is know in the server only?

I think this relates to #10827 (comment) . If we assume if the value is an object, we know to merge edits as partial to what's current saved value.

This may not actually be happening (yet) if one calls getEditedPostAttribute( 'meta' )

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Oct 23, 2018

If we assume if the value is an object, we know to merge edits as partial to what's current saved value.

We can assume the edits are partial for "meta" because that's Core but can we assume it for all post object properties. I think users can register custom properties there.

@aduth

This comment has been minimized.

Member

aduth commented Oct 23, 2018

but can we assume it for all post object properties

I don't know the answer to this. It is a requirement for my proposal to work, yes, and also the intention of the question raised in #10827 (comment) .

@danielbachhuber

This comment has been minimized.

Member

danielbachhuber commented Oct 23, 2018

Should it really matter if we send redundant data? I don't really understand why the REST API should return an error response for noop (re #10204 (comment)).

To be honest, I think this is a bug. I have it on my personal todo list to open a Trac ticket for it.

@aduth

This comment has been minimized.

Member

aduth commented Oct 23, 2018

To be honest, I think this is a bug. I have it on my personal todo list to open a Trac ticket for it.

The same sort of thing also occurs in the new autosave endpoint, during whose development I'd also raised some contest toward.

@aduth

This comment has been minimized.

Member

aduth commented Oct 23, 2018

We can assume the edits are partial for "meta" because that's Core but can we assume it for all post object properties.

Alternatively, maybe it's fine to have a manually curated list of properties for which we want to enable this merging behavior? As a constant in one of the store files.

@aduth aduth referenced this pull request Oct 30, 2018

Merged

Editor: Factor initial edits to own state #11267

0 of 2 tasks complete
@kadamwhite

This comment has been minimized.

Contributor

kadamwhite commented Nov 1, 2018

REST API design question: Is it safe to assume that any values of type object we send with a PUT request would be treated as a patch update (as it's occurring here with meta)?

This is the design intent, yes. As always we may have missed areas where we are internally inconsistent but core REST API endpoints should handle incomplete PUT's as partial updates without affecting unrelated existing data.

@aduth

This comment has been minimized.

Member

aduth commented Nov 1, 2018

Discussed today in Slack (link requires registration):

https://wordpress.slack.com/archives/C02RQC26G/p1541094531087100

If we can assume nested values should always be treated as partial updates, then this branch should be considered complete and ready for review in its current state.

@kadamwhite

This comment has been minimized.

Contributor

kadamwhite commented Nov 2, 2018

If we can assume nested values should always be treated as partial updates

This should be tested :) Assumptions can be dangerous.

@aduth aduth force-pushed the 6505-save-only-changed-meta branch from f37925c to 7b6bccd Nov 8, 2018

@aduth

This comment has been minimized.

Member

aduth commented Nov 8, 2018

I've rebased to resolve conflicts with other recent pull requests which have touched edits state (#11267, #10844).

I started to look at how and where the REST API treats objects, first and foremost for the posts endpoint as it's the one relevant here (though establishing a general pattern could be useful too). With the controllers overloaded support of content fields (title, content, excerpt) as objects, it makes me a bit more hesitant to blindly apply the merging behavior. Furthermore, there's nothing which stops a plugin developer from adding additional fields to the REST API (via register_rest_field).

For now, I might see to limiting this to a subset of known properties (meta).

@aduth

This comment has been minimized.

Member

aduth commented Nov 8, 2018

Also, while the implementation here has treated updates as deeply recursive, this is not in-fact supported in the meta fields updates (top-level key only).

https://github.com/WordPress/WordPress/blob/da7a80d67fea29c2badfc538bfc01c8a585f0cbe/wp-includes/rest-api/fields/class-wp-rest-meta-fields.php#L295-L344

@aduth

This comment has been minimized.

Member

aduth commented Nov 8, 2018

The latest commit 40a96ee significantly scales back the extent of the changes here to simply perform a shallow merge on whitelisted post properties (meta only).

@danielbachhuber danielbachhuber added this to the 4.4 milestone Nov 13, 2018

@youknowriad youknowriad modified the milestones: 4.4, 4.5 Nov 15, 2018

@gziolo

This comment has been minimized.

Member

gziolo commented Nov 19, 2018

Is it important for RC? We need to decide today whether is ready or punt it.

@danielbachhuber

This comment has been minimized.

Member

danielbachhuber commented Nov 19, 2018

I think we should land this PR. It's a pretty glaring issue for any custom block development using post meta.

@danielbachhuber

This comment has been minimized.

Member

danielbachhuber commented Nov 19, 2018

I haven't fully tested it though, so I don't know @aduth's decision to hold on it initially.

@aduth

This comment has been minimized.

Member

aduth commented Nov 19, 2018

This is and has been ready for a final review. Technically I could approve it myself, but the most recent revision is almost entirely of my authoring. 炉\_(銉)_/炉

@gziolo

This comment has been minimized.

Member

gziolo commented Nov 19, 2018

Given that it has a set of unit tests which ensure it works as intended, I would be personally fine to have @aduth giving 馃憤 to this PR. Can someone confirm it solves the issue?

@@ -264,7 +281,7 @@ export const editor = flow( [
( key ) => getPostRawValue( action.post[ key ] );

return reduce( state, ( result, value, key ) => {
if ( value !== getCanonicalValue( key ) ) {
if ( ! isEqual( value, getCanonicalValue( key ) ) ) {

This comment has been minimized.

@aduth

aduth Nov 19, 2018

Member

Granting there's one odd edge case for which this doesn't cover: If updates are applied, individual keys won't be unset unless all meta values are now equal with their new canonical values. e.g.

{ type: 'EDIT_POST', edits: { meta: { a: 1, b: 2 } } }
// { edits: { meta: { a: 1, b: 2 } } }

{ type: 'UPDATE_POST', edits: { meta: { a: 1 } } }
// { edits: { meta: { a: 1, b: 2 } } }

{ type: 'UPDATE_POST', edits: { meta: { a: 1, b: 2 } } }
// { edits: {} }

In practice, I don't think this would ever occur (UPDATE_POST is called on save with all edited values assumed as being the new canonical), and the impact being that it would continue to save edited values which had already been saved before.

The alternative would be an implementation which would individually unset keys and remove the top-level key once empty.

@earnjam

This comment has been minimized.

Contributor

earnjam commented Nov 19, 2018

I can confirm the branch in its current form solves the issue as described in #6505.

I can't approve it though because I'm technically the original PR author, even though it's totally changed from that point.

@aduth

aduth approved these changes Nov 19, 2018

Let's merge it in its current form. My prior noted caveat can be considered separately as an enhancement (I'll create an issue).

@aduth aduth merged commit 6ae6285 into master Nov 19, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@aduth aduth deleted the 6505-save-only-changed-meta branch Nov 19, 2018

@aduth

This comment has been minimized.

Member

aduth commented Nov 19, 2018

My prior noted caveat can be considered separately as an enhancement (I'll create an issue).

See #12065

grey-rsi pushed a commit to OnTheGoSystems/gutenberg that referenced this pull request Nov 22, 2018

Recursively step through edits to track individually changed post meta (
WordPress#10827)

* Only send edited meta values as state to editPost

* Recursively step through edits to granularly update state on EDIT_POST

* Don't recurse for arrays

* Editor: Deeply diff/merge edited property edits/updates

* Editor: Pass meta updates from block as patch

* Editor: Shallow merge on whitelisted post property edits
@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Nov 25, 2018

I suspect this PR broke plugins doing getEditedPostAttribute( 'meta' ) because it won't merge the edited and the non-edited metas when you do this call.

related #12289

@aduth

This comment has been minimized.

Member

aduth commented Nov 26, 2018

I suspect this PR broke plugins doing getEditedPostAttribute( 'meta' ) because it won't merge the edited and the non-edited metas when you do this call.

related #12289

Fix at #12331

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment