Skip to content
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

Update the WP packages with fixes prior to WP 6.2 RC2 #4234

Closed

Conversation

Mamaduka
Copy link
Member

@Mamaduka Mamaduka commented Mar 14, 2023

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

This updated the packages to the latest versions with the latest fixes.

WordPress/gutenberg@f22a3cb

Includes the following changes:

$children,
$block
);
$block_content .= sprintf( $inner_content );
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

* @param bool $is_sub_menu Whether the link is part of a sub-menu.
* @return array Colors CSS classes and inline styles.
*/
function block_core_navigation_link_build_css_colors( $context, $attributes, $is_sub_menu = false ) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Mamaduka Mamaduka self-assigned this Mar 14, 2023
@audrasjb
Copy link
Contributor

Alright, looks good to me.
Note for the Core commit: this also fixes Trac ticket 57883.

@hellofromtonya
Copy link
Contributor

Currently reviewing. There is at least 1 fix (so far) that was a bug introduced in 6.1, not in 6.2.

Copy link
Contributor

@hellofromtonya hellofromtonya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking commit until the review of the fixes is completed. Why? To ensure all fixes are bugs and/or regressions introduced in the 6.2 cycle. There is at least 1 so far from 6.1.

@hellofromtonya
Copy link
Contributor

@Mamaduka I've reviewed each of the Gutenberg PRs to determine if each bug / regression was introduced in the 6.2 cycle.

This bug was not introduced in 6.2:

This fix is removing a PHP function. Was this function introduced in 6.2?

I also left comments on each of these PRs.

@Mamaduka
Copy link
Member Author

Mamaduka commented Mar 14, 2023

@hellofromtonya,

  1. Comments: Fix 'sprintf requires more than 1 params' error - Don't have a strong opinion here. I would love to fix the fatal error, but if it's out of the scope of the RCs, then happy to revert the change.
  2. Navigation Link: Remove color generation code - We can also fix this issue without removing the function and applying changes from this area - https://github.com/WordPress/wordpress-develop/pull/4234/files#diff-e3c55a4e55bda8c2b88525cb3627924cefcfd1966f936ebb3370020eb296be94R110.

@hellofromtonya
Copy link
Contributor

Navigation Link: Remove color generation code - We can also fix this issue without removing the function and applying changes from this area - https://github.com/WordPress/wordpress-develop/pull/4234/files#diff-e3c55a4e55bda8c2b88525cb3627924cefcfd1966f936ebb3370020eb296be94R110.

@Mamaduka Do you when that function was introduced into Core? If during 6.2, then removing is fine. Else, could be deprecated instead if not used and the alternative fix applied.

@hellofromtonya
Copy link
Contributor

Comments: Fix 'sprintf requires more than 1 params' error - Don't have a strong opinion here. I would love to fix the fatal error, but if it's out of the scope of the RCs, then happy to revert the change.

This one has a Trac ticket open for it https://core.trac.wordpress.org/ticket/57883. Let's revert it from this PR and continue discussions in that Trac ticket. Sound good? @Mamaduka

@Mamaduka
Copy link
Member Author

@hellofromtonya, definitely not in 6.2. Here is the PR that introduced the function into the Gutenberg plugin - WordPress/gutenberg#21075, so probably WP 5.5-5.6

@hellofromtonya
Copy link
Contributor

definitely not in 6.2. Here is the PR that introduced the function into the Gutenberg plugin - WordPress/gutenberg#21075, so probably WP 5.5-5.6

Okay thank you @Mamaduka. A hard removal of the function breaks Core's BC (Backwards Compatibility) Promise. It can be deprecated instead or fixed. See my comment here WordPress/gutenberg#48927 (comment)

@hellofromtonya
Copy link
Contributor

This one has a Trac ticket open for it https://core.trac.wordpress.org/ticket/57883. Let's revert it from this PR and continue discussions in that Trac ticket.

Sorry for misguidance here. @Mamaduka Let's leave this fix on this PR for now but make it a separate commit please. That way its commit can be excluded if there's a decision not to commit it.

@Mamaduka
Copy link
Member Author

Mamaduka commented Mar 14, 2023

@Mamaduka
Copy link
Member Author

Let's leave this fix on this PR for now but make it a separate commit please. That way its commit can be excluded if there's a decision not to commit it.

@hellofromtonya, it needs to be in a separate package update; otherwise build process will overwrite the changes.

Can we include it in RC3 when the decision is reached?

@hellofromtonya
Copy link
Contributor

Published package updates WordPress/gutenberg@356298f

Copy link
Contributor

@hellofromtonya hellofromtonya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Confirmed the package version numbers match the 2 publish releases WordPress/gutenberg@f22a3cb and WordPress/gutenberg@356298f
  • Confirmed the removed PHP function is restored to avoid breaking BC ✅
  • Confirmed the sprintf() bug introduced in 6.1.0 is not in this package update as it is still being discussed ✅

Ready for commit 👍

@hellofromtonya
Copy link
Contributor

Committed via https://core.trac.wordpress.org/changeset/55542 into trunk.

@Mamaduka Mamaduka deleted the update/packages-wp62-pre-rc2 branch May 9, 2023 09:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants