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

Elements API updates backport #3206

Conversation

MaggieCabrera
Copy link

@MaggieCabrera MaggieCabrera commented Sep 7, 2022

This PR backports the following changes:

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


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.

@ockham ockham mentioned this pull request Sep 7, 2022
@MaggieCabrera
Copy link
Author

While trying to fix the tests I found a bug that wasn't caught in GB because the test is only on core, not in the GB repo. I'm going to try and patch that so we can add it to this one or we could "hack" the snapshot and then patch afterwards. I'm more inclined to do the former.

@MaggieCabrera
Copy link
Author

MaggieCabrera commented Sep 8, 2022

This is the fix for the issue that the tests uncovered.

Edit: merged and backported here too

MaggieCabrera and others added 21 commits September 9, 2022 10:14
… #40889

Co-authored-by: Ben Dwyer <ben@scruffian.com>
Co-authored-by: Ben Dwyer <ben@scruffian.com>
Co-authored-by: Ben Dwyer <ben@scruffian.com>
Co-authored-by: Dave Smith <getdavemail@gmail.com>
Co-authored-by: Adam Zielinski <adam@adamziel.com>
Co-authored-by: Ben Dwyer <ben@scruffian.com>
Co-authored-by: George Mamadashvili <georgemamadashvili@gmail.com>
Co-authored-by: Ben Dwyer <ben@scruffian.com>
Co-authored-by: Andrei Draganescu <andrei.draganescu@automattic.com>
Co-authored-by: Ben Dwyer <ben@scruffian.com>
Co-authored-by: Dave Smith <getdavemail@gmail.com>
Co-authored-by: Adam Zielinski <adam@adamziel.com>
Co-authored-by: Ben Dwyer <ben@scruffian.com>
Co-authored-by: David Calhoun <438664+dcalhoun@users.noreply.github.com>
…2072

Co-authored-by: Dave Smith <getdavemail@gmail.com>
Co-authored-by: Dave Smith <getdavemail@gmail.com>
… #42669

Co-authored-by: Ben Dwyer <ben@scruffian.com>
Co-authored-by: Ben Dwyer <ben@scruffian.com>
Co-authored-by: George Mamadashvili <georgemamadashvili@gmail.com>
…blocks on public pages (#41335)

* Adding  the code  existing in a previous version of this file, inside this loop:  https://github.com/WordPress/gutenberg/blob/9c1c67bba4ba0dc7b241639719c567a364a8ea05/lib/compat/wordpress-6.0/class-wp-theme-json-6-0.php#L320

* Avoid repeating code

* linting
Co-authored-by: Glen Davies <glen.davies@a8c.com>
Co-authored-by: Ben Dwyer <ben@scruffian.com>
@MaggieCabrera MaggieCabrera force-pushed the backport-elements-api-changes branch from 32d5e2c to a7283ec Compare September 9, 2022 08:29
@MaggieCabrera MaggieCabrera marked this pull request as ready for review September 9, 2022 10:13
@MaggieCabrera
Copy link
Author

this only needs a review and check what's breaking the tests (they are all passing locally)

@SergeyBiryukov
Copy link
Member

The tests previously failed on PHP 5.6 with a fatal error:
Fatal error: Cannot use isset() on the result of an expression (you can use "null !== expression" instead) in src/wp-includes/class-wp-theme-json.php on line 1526

0015d99 should resolve the issue. We already used array_key_exists( $element, static::VALID_ELEMENT_PSEUDO_SELECTORS ) in one place, and that resolves the error in other instances in my testing.

Copy link
Member

@SergeyBiryukov SergeyBiryukov left a comment

Choose a reason for hiding this comment

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

This looks good to me 👍 Made some minor adjustments to documentation and unit tests for consistency with the rest of core. Going to merge shortly.

@SergeyBiryukov
Copy link
Member

Thanks for the PR! Merged in r54118.

@ockham
Copy link
Contributor

ockham commented Sep 11, 2022

Thanks a lot, folks!

I was alerted that unfortunately, r54118 caused a fatal error on websites running Core trunk and a current version of the Gutenberg plugin 😕

PHP Fatal error:  Cannot redeclare wp_add_global_styles_for_blocks() (previously declared in wp-includes/global-styles-and-settings.php:202) in wp-content/plugins/gutenberg/lib/compat/wordpress-6.1/get-global-styles-and-settings.php on line 12

We'll need to add a ! function_exists() wrapper around the function definition in Gutenberg to avoid this.

Unfortunately, this won't fix the problem immediately: WP sites running Core trunk with the GB plugin active will continue to be affected until a new version of the plugin is released (as planned for next Thursday); or until the change is merged into GB trunk (if the site is running GB trunk). And finally, sites running Core trunk and an older version of GB (that has the unguarded wp_add_global_styles_for_blocks function definition) will continue to be affected 😕

I'm not sure how to best cover the latter case. For mitigation, we might want to consider reverting r54118 (as much as it pains me to suggest that) -- at least until a GB version with the fix is released.


Going forward, we should probably put some strategies in place to avoid collisions like this:

  • Require wp_ function definitions in GB to be guarded with a ! function_exists() check (and ideally enforce with a WPCS lint).
  • Add a check in wordpress-develop's CI to install and enable GB alongside Core (and run some basic tests to make sure it doesn’t fatal).

Anyway, this is something we'll discuss separately. I'll file one or two issues tomorrow.

@ockham
Copy link
Contributor

ockham commented Sep 12, 2022

Update: Unbeknownst to me, the wp_ prefix has been changed to gutenberg_ in the GB plugin: WordPress/gutenberg#44052

@ockham
Copy link
Contributor

ockham commented Sep 12, 2022

Update #2: Let's hold off on the revert. We might release a Gutenberg point release with this fix as a stop-gap.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

5 participants