Make WordPress Core

Opened 9 months ago

Closed 9 months ago

Last modified 9 months ago

#58885 closed defect (bug) (fixed)

Update @wordpress npm packages for 6.3 RC2

Reported by: isabel_brison's profile isabel_brison Owned by: audrasjb's profile audrasjb
Milestone: 6.3 Priority: normal
Severity: normal Version: 6.3
Component: Editor Keywords: gutenberg-merge has-patch fixed-major
Focuses: Cc:

Description

The @wordpress npm packages have been republished with the bugfixes detailed in this PR: https://github.com/WordPress/gutenberg/pull/52863 so they need to be updated in core before RC2.

Change History (22)

This ticket was mentioned in PR #4892 on WordPress/wordpress-develop by @isabel_brison.


9 months ago
#1

  • Keywords has-patch added

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

Updated packages with bugfixes from https://github.com/WordPress/gutenberg/pull/52863.

This PR also includes the fix from #4871, so we may not need to commit that one to the release branch separately.

@isabel_brison commented on PR #4892:


9 months ago
#2

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

joemcgill commented on PR #4892:


9 months ago
#3

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?

@isabel_brison commented on PR #4892:


9 months ago
#4

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.

@ramonopoly commented on PR #4892:


9 months ago
#6

Further fixes from https://github.com/WordPress/gutenberg/pull/52915 will be updated in the packages

Packages (wp/6.3) are ready for sync

@isabel_brison commented on PR #4892:


9 months ago
#7

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

#8 @isabel_brison
9 months ago

  • Keywords dev-feedback added

@mikeschroder commented on PR #4892:


9 months ago
#9

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?

@swissspidy commented on PR #4892:


9 months ago
#10

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 https://github.com/WordPress/gutenberg/pull/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 https://github.com/WordPress/gutenberg/issues/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.

@mikeschroder commented on PR #4892:


9 months ago
#11

cc: @ellatrix for any additional feedback on the couple concerns / topics being talked about

#12 @isabel_brison
9 months ago

Note that this commit: https://core.trac.wordpress.org/changeset/56298 should have been attached to this ticket (I added the wrong ticket number to it)

@isabel_brison commented on PR #4892:


9 months ago
#13

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.

#14 @audrasjb
9 months ago

  • Keywords fixed-major added

#15 @audrasjb
9 months ago

  • Keywords dev-reviewed added; dev-feedback removed

[56298] is approved for backport to branch 6.3 on my side 👍

#16 @audrasjb
9 months ago

  • Keywords dev-feedback added; dev-reviewed removed

Removing dev-reviewed since there is still a ongoing discussion in the related PR.

#17 @kirasong
9 months ago

I'm heading out soon as well for the night, but wanted to leave a note here first:

I'm also fine with this getting backported for 6.3 RC2.

I think it's good to have the conversation that's happening, but not at the expense of the package update + set of fixes missing the RC2 deadline.

We can make additional changes after RC2 ships if it's needed / there is consensus later.

@ellatrix commented on PR #4892:


9 months ago
#18

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

#19 @audrasjb
9 months ago

Self assigning for 6.3 backport / reviewed by @mikeschroder.

#20 @audrasjb
9 months ago

  • Owner set to audrasjb
  • Status changed from new to accepted

#21 @audrasjb
9 months ago

  • Keywords dev-feedback removed
  • Resolution set to fixed
  • Status changed from accepted to closed

This was backported in changeset [56305].

(I copied the trunk commit message and it appears the initial Trac ticket mention was wrong)

@isabel_brison commented on PR #4892:


9 months ago
#22

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

Note: See TracTickets for help on using tickets.