Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add revisioning of post meta, including ‘footnotes’ by default #4859
Add revisioning of post meta, including ‘footnotes’ by default #4859
Changes from 56 commits
b8bf5ef
6667610
15e9e4e
b892323
6235af5
3a87749
05d0f86
19e1f8a
0b86da9
52dd943
ef14c11
31a6d61
da8abe6
d2f0db1
6c9bece
2e74964
268385f
4fd4302
3722d1e
b26a695
5d6e5b9
b4fa33f
1603c55
d244a39
d86ec77
a80620a
885640d
ea4f181
e63334b
f4234f8
1f1e5a0
6c5061b
5bd7e7b
65bfe81
7254e3a
8c3fba2
26a9d0b
210573b
485f20b
167261e
5b5dc0d
f8d0d30
ea9b8ce
15e8bbd
1e54aa3
a3fa561
877420e
f57b79b
cad9272
57c872a
ca01058
92590aa
6fd9a9e
f83d353
7f70990
432a810
d80b7b4
7160abb
96301da
e2d0cc7
5f07145
afc1109
ac279e3
4205d65
e7b4495
68f6410
ee35085
61bfafc
c27258a
20874e3
68a419b
3ee633f
1e17f5b
982c553
d04574a
3a8f6fc
192e7c0
225a91b
c2482fd
2331d36
5c48c7a
e211ca1
7247e93
891a0e4
c35cc5f
826bebb
750dcdc
0b88e50
2c02dbc
b5ba2d3
95508d9
b0b880b
f04a319
e231886
e1950b0
6f6e3ce
b77bf22
b6e221a
40febf2
69c6107
f9b760b
8360faa
a1971f7
99b2aa8
b94e1a6
e09f011
01a4788
8e7b72a
d6c6ca2
73f6a23
3db644c
95c9e9d
9bab9a9
dc0b907
ab2d500
8bf1727
c7904ce
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I like this. The root of the problem is that the order of filters and actions in this REST API request is wrong.
wp_insert_post
andwp_update_post
fire thepost_updated
action after meta is updated (throughmeta_input
). The REST API should do the same. Why does the REST API only fix thewp_after_insert_post
to fire after meta is updated? It should do the same forpost_updated
and all those other filters.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know who else has recently worked on these endpoints. Maybe @TimothyBJacobs, @kadamwhite?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm also not sure why the REST API decides not to use
meta_input
. Probably because it has limitation, but then those limitations should be addressed? Ideally everything goes through one code path. Since the REST API updates meta by itself, it had to move thewp_after_insert_post
action, and will now have to do the same for all other actions.So either we have to move those other actions, or use
meta_input
and address whatever the reasons were for not using that.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The meta_input parameter is poorly designed for doing anything of complexity. It can't handle multi meta and support can't easily be added in a BC manner. It doesn't handle deleting meta. It doesn't provide any error handling.
Perhaps the REST API could've been architected differently, but that ship has long since sailed.
The REST API didn't move that hook, we introduced that hook specifically for the REST API. Core should be moving appropriate actions to that hook.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not because something has been poorly designed that it can't be improved? Is there something fundamentally unfixable about
meta_input
?Core could move actions to that hook, but that ship has also sailed? We'd need to make sure plugins removing these hooks are still removing the moved hooks somehow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ellatrix I also agree this solution isn't fantastic, it is a bit of a compromise approach. The one part I do like is hooking
wp_save_revisioned_meta_fields
on an action which lets any plugin easily disable the feature entirely.I don't quite understand the part about
meta_input
. Are you saying the REST endpoint could populate that before updating the post? What would be a more ideal solution in your mind?I am open to considering changing this - of course we would need to be careful not to break any cases where plugins already expect the current behavior. Also, this change could easily be done after this PR lands, so I don't feel it should be a blocker here. We would still hook the revisioning as is, we would just remove the special unhooking/re-hooking logic in the posts controller if the order was fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I mean is that there's a
meta
property that can be passed to the post data on theposts
endpoint, which is essentially the same asmeta_input
that can be passed to the post data forwp_insert_post
andwp_update_post
. So there's great potential for symmetry here. I don't get the argument of it being poorly designed for complex things. The input for both should be almost the same: a keys to values array. So you're telling me that whatever handling the REST API is doing for meta, cannot be done inside ofwp_update_post
? Why can't we add deletion and error handling? If we do that, the action order will be correct and all will be well?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm good point! I will test out making the change you are suggesting, I'm not sure why that wouldn't be possible maybe I am missing something and @TimothyBJacobs can clarify. I do see that there are other changes also completed after
wp_update_post
, but I don't see why we can't just include the meta inwp_update_post
asmeta_input
and - as you say - all will be well.