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 GitHub Actions failure due to duplicate functions #44052

Merged
merged 3 commits into from
Sep 12, 2022

Conversation

t-hamano
Copy link
Contributor

@t-hamano t-hamano commented Sep 10, 2022

What?

This PR checks for redeclarations in the wp_add_global_styles_for_blocks function and resolves Fatal errors in gitHub Actions.
In some actions, the following Fatal Error occurs:

Fatal error: Cannot redeclare wp_add_global_styles_for_blocks() (previously declared in /var/www/html/wp-includes/global-styles-and-settings.php:202) in /var/www/html/wp-content/plugins/plugin/lib/compat/wordpress-6.1/get-global-styles-and-settings.php on line 11

Why?

A backport was done on Changeset 54118 in WordPress core and wp_add_global_styles_for_blocks was added.
I believe this problem occurred because the function is not checked on the Gutenberg.

How?

I followed this best practice and added checks by function_exists.

Testing Instructions

In this PR, confirm that all tests pass.
Confrim that all tests pass, except for the performance test where trunk is used.

@t-hamano t-hamano added the Backwards Compatibility Issues or PRs that impact backwards compatability label Sep 10, 2022
@t-hamano t-hamano self-assigned this Sep 10, 2022
@t-hamano t-hamano marked this pull request as ready for review September 10, 2022 14:43
@t-hamano t-hamano changed the title Add redeclaration check of wp_add_global_styles_for_blocks Fix GitHub Actions failure due to duplicate functions. Sep 10, 2022
@t-hamano t-hamano changed the title Fix GitHub Actions failure due to duplicate functions. Fix GitHub Actions failure due to duplicate functions Sep 10, 2022
@amustaque97
Copy link
Member

I'm not sure but the Performance Tests/Run performance tests stage is failing because of the same reason. failure link: https://github.com/WordPress/gutenberg/runs/8285679201?check_suite_focus=true#step:5:60

Could you please check it again?

Thank you

@t-hamano
Copy link
Contributor Author

My understanding is that the performance test will not pass the test unless it is merged because trunk is also used.

Copy link
Member

@amustaque97 amustaque97 left a comment

Choose a reason for hiding this comment

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

I tested your changes locally (I tried in my fork version but it was a bit complex on Github so I left it there 😅 ). Performance test step doesn't throw any re-declaration exceptions now. ➕ 🚀

Copy link
Contributor

@andrewserong andrewserong left a comment

Choose a reason for hiding this comment

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

Thanks for looking into a fix @t-hamano! For this function, I'm wondering if instead we should be using a gutenberg prefixed version instead of attempting to use the version that's just been merged into core?

Comment on lines 8 to 12
if ( ! function_exists( 'wp_add_global_styles_for_blocks' ) ) {
/**
* Adds global style rules to the inline style for each block.
*/
function wp_add_global_styles_for_blocks() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of checking whether or not the core wp_add_global_styles_for_blocks exists, should we instead rename the function to gutenberg_add_global_styles_for_blocks so that in the plugin, we're always calling the Gutenberg version? This would make it similar to the function lower down in this file (gutenberg_get_global_stylesheet).

Comment on lines 159 to 161
if ( ! function_exists( 'wp_add_global_styles_for_blocks' ) ) {
wp_add_global_styles_for_blocks();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This check appears to call the function only if it doesn't exist.

Instead, should we use a gutenberg prefixed version of the function?

@t-hamano
Copy link
Contributor Author

t-hamano commented Sep 12, 2022

Thanks for the review, @andrewserong !
I have made the change to use the gutenberg_ prefix.

Copy link
Contributor

@andrewserong andrewserong left a comment

Choose a reason for hiding this comment

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

Thanks for following up @t-hamano, this looks good to me, and styles appear to be output correctly on my test site.

Now we just need someone with access to merge this in 😄

@noisysocks noisysocks merged commit 53d19c2 into trunk Sep 12, 2022
@noisysocks noisysocks deleted the fix/redeclare-function-6.1 branch September 12, 2022 01:34
@andrewserong
Copy link
Contributor

Thanks for merging, @noisysocks! 🙇

@ockham
Copy link
Contributor

ockham commented Sep 12, 2022

Thank you for addressing, @t-hamano and @andrewserong!

Here's some more discussion on the issue: WordPress/wordpress-develop#3206 (comment)

@ockham
Copy link
Contributor

ockham commented Sep 12, 2022

I'm going to release GB 14.0.3 in a bit to include this fix, in order to make it possible to run GB stable against Core trunk.

ockham pushed a commit that referenced this pull request Sep 12, 2022
* Add redeclaration check of `wp_add_global_styles_for_blocks`

* Add skip redirection

* Use gutenberg_ prefix
@ockham ockham modified the milestones: Gutenberg 14.1, Gutenberg 14.0 Sep 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backwards Compatibility Issues or PRs that impact backwards compatability
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants