Update `@wordpress` packages for 6.3 RC2 by tellthemachines · Pull Request #4892 · WordPress/wordpress-develop

tellthemachines · 2023-07-24T03:30:56Z

Trac ticket: https://core.trac.wordpress.org/ticket/58885

Updated packages with bugfixes from WordPress/gutenberg#52863 and WordPress/gutenberg#52915.

This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

ramonjd

I went through the commit list in WordPress/gutenberg#52863 and tested functionality.

LGTM

tellthemachines · 2023-07-24T07:29:16Z

Unit test failures seem to be due to this change; looking into it

audrasjb

THanks for the PR!
I have two docblock changes :)

audrasjb · 2023-07-24T09:41:38Z

src/wp-includes/blocks/footnotes.php

+	'_wp_put_post_revision',
+	/**
+	 * Keeps track of the revision ID for "rest_after_insert_{$post_type}".
+	 *


It's missing a since declaration here :)

Ah, so anonymous functions should also receive @since?

Why are these all anonymous functions in the first place?

Makes it impossible for developers to unhook them.

I personally like anonymous functions for handling the footnote meta because it makes the way to move to handling postmeta revisions in a more standard way in 6.4.

Unless you think we'd still want the named functions after doing so?

That would indeed be an argument for keeping them, but not sure if that's the reason for it?

Because this is a temporary fix and to avoid having to rename/naming clashes

Does this seem like an appropriate reason for using anonymous functions to you, @swissspidy ? (+cc: @peterwilsoncc and @joemcgill ).

If not, do you think we should continue discussion about this in the existing issue about revision support or open a new issue?

It sounded like there may be platform considerations for using named functions here, and I want to make sure those are well understood.

src/wp-includes/blocks/footnotes.php

ramonjd · 2023-07-24T10:03:34Z

src/wp-includes/blocks/footnotes.php

+	 * @param int $revision_id The revision ID.
+	 */
+	static function( $revision_id ) {
+		global $_gutenberg_revision_id;


What about the gutenberg prefix here. Is that okay?

Also quite a hacky workaround in general having to use such a global here :/

I understand this is a temporary situation until revisions properly support post meta in core.

Open for any better workarounds. It's quite late now to be adding general post meta revision support or start changing the REST API hook order.

ramonjd · 2023-07-24T10:05:41Z

src/wp-includes/blocks/footnotes.php

+			global $_gutenberg_revision_id;
+
+			if ( $_gutenberg_revision_id ) {
+				$revision = get_post( $_gutenberg_revision_id );


Wondering if we should use wp_get_post_revision here? It implements get_post, but has extra revision-specific checks that get_post doesn't.

Well, namely

if ( 'revision' !== $revision->post_type ) { return null; }

😄

Answered here: WordPress/gutenberg#52870 (comment)

joemcgill · 2023-07-24T16:27:15Z

I agree with the feedback about the way revisions are being handled in this PR not being ideal. However, I assume these changes will need to be made in the Gutenberg repo so they don't get overwritten during a future sync?

peterwilsoncc

I agree with @swissspidy that it would be preferable to move the anonymous functions to named functions to allow developers to unhook them.

If this requires changes to the Gutenberg repo, then I think it's fine to commit as is for the purposes of this week's RC to allow a little extra time to get the upstream patches in.

tellthemachines · 2023-07-25T01:14:43Z

Thanks for the feedback folks! We have to make a change in gutenberg already to address the PHP test errors, so I think we can do the renaming while we're at it.

ramonjd · 2023-07-25T06:52:33Z

Further fixes from WordPress/gutenberg#52915 will be updated in the packages

ramonjd · 2023-07-25T07:39:41Z

Further fixes from WordPress/gutenberg#52915 will be updated in the packages

Packages (wp/6.3) are ready for sync

tellthemachines · 2023-07-25T08:18:24Z

Ok folks I think we've addressed all the feedback now!

feedback has been addressed

ocean90 · 2023-07-25T09:26:23Z

src/wp-includes/blocks/footnotes.php

+	}
+}
+
+foreach ( array( 'post', 'page' ) as $post_type ) {


Shouldn't this support all post types using the block editor and/or have revision support enabled?

I think this conversation / issue may be related:
WordPress/gutenberg#52812

WordPress/gutenberg#52812 (comment)

getsource · 2023-07-25T09:28:05Z

I left this further up in the review, but thought it should be here as well so it ends up in the ticket.

My understanding was that the anonymous functions were being used to help with a move to handling postmeta revisions in a more standard way in 6.4.

Do y'all think we'd still want the named functions after doing so?
If not, is there an approach that would allow the developer control necessary + removing them in the future?

Edit: To be clear, given committer support above, I am not intending this comment as a blocker for merge for RC2, but for clarification + so the conversation happens.

swissspidy · 2023-07-25T10:02:57Z

My understanding was that the anonymous functions were being used to help with a move to handling postmeta revisions in a more standard way in 6.4.

Maybe, but there's no explanation at WordPress/gutenberg#52686 from what I can see.

Do y'all think we'd still want the named functions after doing so?

If there's no concern with back compat / tech debt, I suppose we can keep them anonymous until 6.4 lands 👍

Another thing I find very concerning in general is that this post meta revision support is a late-stage enhancement with no unit tests at all.

Not to mention WordPress/gutenberg#52812

Right now I tend to agree with this comment by Ella:

this bug makes me think that footnotes is not ready for 6.3.

getsource · 2023-07-25T10:36:13Z

^ cc: @ellatrix in case you want to give any additional feedback on the couple concerns / topics being talked about

tellthemachines · 2023-07-25T10:56:23Z

Another thing I find very concerning in general is that this post meta revision support is a late-stage enhancement with no unit tests at all.

I'm considering this to be a bug fix, not an enhancement, because it would not be expected that footnotes stay unchanged when reverting to a previous revision. The fact that this happens because footnotes are tied to post meta is an implementation detail; the behaviour is still buggy. I agree that it would be good to have some unit tests though.

Regarding the functions: I changed them to named functions based on the previous feedback. I'm not too concerned one way or the other as I guess the worst case is they'll have to be deprecated once proper support for post meta in revisions is implemented. But changing them back at this point will require a Gutenberg PR and re-publishing the npm packages, and my bedtime is fast approaching 😅 so I'll have to leave that for someone else to do.

Note that I've already committed this changeset to trunk here.

ellatrix · 2023-07-25T12:36:38Z

There is an e2e test for revisions. We can add more flows if anyone sees a need.

tellthemachines · 2023-07-27T01:03:48Z

Closing this one as it landed in the release branch in https://core.trac.wordpress.org/changeset/56305.

ramonjd approved these changes Jul 24, 2023

View reviewed changes

tellthemachines mentioned this pull request Jul 24, 2023

Check if object exists before accessing its properties. WordPress/gutenberg#52870

Merged

audrasjb previously requested changes Jul 24, 2023

View reviewed changes

ramonjd reviewed Jul 24, 2023

View reviewed changes

peterwilsoncc reviewed Jul 24, 2023

View reviewed changes

ramonjd mentioned this pull request Jul 25, 2023

Footnotes: fix PHP unit failures for revision handling WordPress/gutenberg#52879

Closed

Update @wordpress packages for 6.3 RC2

84f2b13

tellthemachines force-pushed the update/packages-6-3-rc2 branch from dddca08 to 84f2b13 Compare July 25, 2023 08:05

Update packages with latest bugfixes

b5ba0d4

tellthemachines requested a review from audrasjb July 25, 2023 08:17

ocean90 reviewed Jul 25, 2023

View reviewed changes

tellthemachines closed this Jul 27, 2023

Conversation

tellthemachines commented Jul 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ramonjd left a comment

Choose a reason for hiding this comment

Uh oh!

tellthemachines commented Jul 24, 2023

Uh oh!

audrasjb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joemcgill commented Jul 24, 2023

Uh oh!

peterwilsoncc left a comment

Choose a reason for hiding this comment

Uh oh!

tellthemachines commented Jul 25, 2023

Uh oh!

ramonjd commented Jul 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ramonjd commented Jul 25, 2023

Uh oh!

tellthemachines commented Jul 25, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

getsource commented Jul 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

swissspidy commented Jul 25, 2023

Uh oh!

getsource commented Jul 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tellthemachines commented Jul 25, 2023

Uh oh!

ellatrix commented Jul 25, 2023

Uh oh!

tellthemachines commented Jul 27, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

tellthemachines commented Jul 24, 2023 •

edited

Loading

ramonjd commented Jul 25, 2023 •

edited

Loading

getsource commented Jul 25, 2023 •

edited

Loading

getsource commented Jul 25, 2023 •

edited

Loading