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

Fix Gutenberg 11.8.2 in WordPress trunk #36347

Merged
merged 5 commits into from
Nov 9, 2021

Conversation

oandregal
Copy link
Member

@oandregal oandregal commented Nov 9, 2021

Note this PR has release/11.8 as its base branch.

Activating the Gutenberg 11.8.2 plugin with the latest WordPress core trunk causes an error due to some name clashes:

  • GlobalStyles code: WP_Theme_JSON_Schema (this PR renames it to be WP_Theme_JSON_Schema_Gutenberg)
  • Calendar block: block_core_calendar_update_has_published_post_on_delete and block_core_calendar_update_has_published_post_on_transition_post_status already exist in core, so this PR only defines them if they don't exist.

Note that Gutenberg 11.9 is expected to be released tomorrow, so that should be the preferred fix. This PR only exists to help with a potential 11.8.3, should it happen. See conversation in core-editor slack (requires registration).

cc @Mamaduka @priethor

How to test

  • Upload the plugin in this branch to a WordPress core environment.
  • Test that the plugin can be activated and operated normally (without this fix it will raise an error upon activation).

@oandregal oandregal changed the title Fix Gutenberg 11.8.2 with core trunk Fix Gutenberg 11.8.2 with WordPress trunk Nov 9, 2021
@oandregal oandregal changed the title Fix Gutenberg 11.8.2 with WordPress trunk Fix Gutenberg 11.8.2 in WordPress trunk Nov 9, 2021
@@ -115,7 +115,7 @@ function block_core_calendar_update_has_published_posts() {
*
* @param int $post_id Deleted post ID.
*/
function block_core_calendar_update_has_published_post_on_delete( $post_id ) {
function gutenberg_block_core_calendar_update_has_published_post_on_delete( $post_id ) {
Copy link
Member

Choose a reason for hiding this comment

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

This can be a problem. I think block PHP files are automatically copied into WP core during package updates, and we can't have the gutenberg_ prefix there.

Copy link
Member Author

Choose a reason for hiding this comment

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

🤔 This and the other change in this file were causing fatals as well. What would be the proper fix?

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I'm not sure. I know Nik (@ntsekouras) renamed a bunch of functions last week, maybe he knows what's the proper fix.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think block PHP files are automatically copied into WP core during package updates, and we can't have the gutenberg_ prefix there.

That's correct. How this function creates any problem as is only used from this block? It would be a problem if it did call a gutenberg prefixed function that made it into core (example PR.

Copy link
Member Author

@oandregal oandregal Nov 9, 2021

Choose a reason for hiding this comment

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

Hey, this is what I know. Without these fixes I get this error when activating the plugin (11.8.2 version) with the latest WordPress core:

Captura de ecrã de 2021-11-09 11-38-45

I also see that the functions block_core_calendar_update_has_published_post_on_transition_post_status and block_core_calendar_update_has_published_post_on_delete were backported to WordPress core as of this commit (see related PR).

So, the fact that the plugin uses the same names is why the error happens, and renaming them fixes it. What I'm unfamiliar with is how packages/block-library/src/calendar/index.php in Gutenberg is converted into the src/wp-includes/blocks/calendar.php#L151 in WordPress core, but I presume is part of the package update. In any other file, I'd rename the functions to something different. How do we update this kind of files?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm changing the block calendar code to cover against the function being already present at 601a012 so it doesn't require a package update. Props @jorgefilipecosta

Copy link
Contributor

Choose a reason for hiding this comment

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

Removed the conditional functions to avoid issues with the build step of Gutenberg that renames the functions in block files.

@youknowriad youknowriad added the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Nov 9, 2021
@youknowriad
Copy link
Contributor

The failures happening here are already happening in the release branch 11.8, I wonder why they're not happening on trunk, maybe there's something else that need to be backported

@youknowriad
Copy link
Contributor

Anyway, I guess It shouldn't block this PR.

@jorgefilipecosta jorgefilipecosta merged commit e9c3607 into release/11.8 Nov 9, 2021
@jorgefilipecosta jorgefilipecosta deleted the fix/11-8-with-core-trunk branch November 9, 2021 15:10
@noisysocks
Copy link
Member

@youknowriad: I'm confused, which bit of this did you want to backport to Core? The base is release/11.8 so it won't cherry pick cleanly.

@youknowriad
Copy link
Contributor

I thought this one was also meant to be cherry-picked to trunk and wp/trunk branches, that's about it. There's no need for php changes in Core itself.

@noisysocks noisysocks removed the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Nov 15, 2021
noisysocks pushed a commit that referenced this pull request Nov 15, 2021
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.

6 participants