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

get_style_nodes should be compatible with parent method. #41217

Merged
merged 4 commits into from
May 23, 2022

Conversation

torounit
Copy link
Member

What?

Fix the following errors and fix phpdoc comment for get_styles_for_block.
 

Warning: Declaration of WP_Theme_JSON_6_1::get_style_nodes($theme_json) should be compatible with WP_Theme_JSON_5_9::get_style_nodes($theme_json, $selectors = Array) in /var/www/html/wp-content/plugins/gutenberg/lib/compat/wordpress-6.1/class-wp-theme-json-6-1.php on line 124

Testing Instructions

  1. npm run wp-env start
  2. show http://localhost:8888/

@torounit torounit added the [Type] Bug An existing feature does not function as intended label May 22, 2022
@ramonjd
Copy link
Member

ramonjd commented May 23, 2022

Thanks for adding this patch @torounit

I just added some minor formatting changes to kick the CI tests off again 😄

The changes in 323174a#diff-f28aa79997745a4a51f917012b45a809c9b45d55134b55379a718f5c751dddf4R160 removed the use of $selectors in get_style_nodes(), so now it's an unused parameter and will trigger the lint warning.

124 | WARNING | Unused function parameter $selectors.

Do we throw in an ignore comment to suppress the warning?

protected static function get_style_nodes( $theme_json, $selectors = array() ) { //phpcs:ignore VariableAnalysis.CodeAnalysis.VariableAnalysis.UnusedVariable
...

That would be an interim measure only, and not ideal.

I wonder if we should rather wind back the optimization to dedupe the code in get_block_nodes?

Basically we put up with duplicated code in get_style_nodes() to retain the same function signature.

The other, harder way, might be to create a new function and deprecate get_style_nodes(). That might involve changing more files wherever get_style_nodes() is called to remove the second parameter.

What do you think @scruffian ?

cc @adamziel @draganescu

@ramonjd
Copy link
Member

ramonjd commented May 23, 2022

Actually, passing an optional $selectors arg to get_block_nodes might do the trick. 😆

@ramonjd
Copy link
Member

ramonjd commented May 23, 2022

I threw up a PR over in #41218 in order to add a check that avoids the PHPCodeSniffer error

Fatal error: Uncaught TypeError: vsprintf(): Argument #2 ($values) must be of type array, string given in /app/vendor/squizlabs/php_codesniffer/src/Files/File.php:1056

Co-authored-by: Andrew Serong <14988353+andrewserong@users.noreply.github.com>
@ramonjd
Copy link
Member

ramonjd commented May 23, 2022

I threw up a PR over in #41218 in order to add a check that avoids the PHPCodeSniffer error

Thanks for spotting that space in bebac4c @andrewserong

🤞 this one passes. I'll close my other test PR.

Copy link
Member

@ramonjd ramonjd left a comment

Choose a reason for hiding this comment

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

LGTM!

I tested according to the instructions over in #41160

Thanks for putting this PR together @torounit !

@ramonjd ramonjd merged commit 78cea0f into trunk May 23, 2022
@ramonjd ramonjd deleted the fix/get_style_nodes-compatible branch May 23, 2022 03:03
@github-actions github-actions bot added this to the Gutenberg 13.4 milestone May 23, 2022
@draganescu
Copy link
Contributor

Missed the ping but this is 👌🏻 For a moment I wondered if we want to remove $selectors when this is merged, or do we still want it in get_style_nodes? It appears get_block_nodes needs it? get_stylesheet passes $blocks_metadata but that only is required on get_block_nodes which could also get it from static::get_blocks_metadata() so it seems useless?

westonruter added a commit that referenced this pull request May 23, 2022
…p-tests-config

* 'trunk' of github.com:WordPress/gutenberg: (88 commits)
  Components: refactor `AlignmentMatrixControl` to pass `exhaustive-deps` (#41167)
  [RNMobile] Add 'Insert from URL' option to Image block (#40334)
  [RNMobile] Improvements to Getting Started Guides (#40964)
  Post Author Name: Add to and from Post Author transformations (#41151)
  CheckboxControl: Add unit tests (#41165)
  Improve inline documentation (#41209)
  Mobile Release v1.76.1 (#41196)
  Use explicit type definitions for entity configuration (#40995)
  Scripts: Convert file extension to js in `block.json` during build (#41068)
  Reflects revert in 6446878 (#41221)
  get_style_nodes should be compatible with parent method. (#41217)
  Gallery: Opt-in to axial (column/row) block spacing controls (#41175)
  Table of Contents block: convert line breaks to spaces in headings. (#41206)
  Add support for button elements to theme.json (#40260)
  Global Styles: Load block CSS conditionally (#41160)
  Update URL (#41188)
  Improve autocompleter performance (#41197)
  Site Editor: Set min-width for styles preview (#41198)
  Remove Navigation Editor screen from experiments page (#40878)
  Fix broken Page title for pages created inline within in Nav block (#41063)
  ...
@ramonjd
Copy link
Member

ramonjd commented May 24, 2022

For a moment I wondered if we want to remove $selectors when this is merged, or do we still want it in get_style_nodes? It appears get_block_nodes needs it? get_stylesheet passes $blocks_metadata but that only is required on get_block_nodes which could also get it from static::get_blocks_metadata() so it seems useless

Thanks @draganescu

You're right on both counts: we don't need $selectors in get_style_nodes() anymore. static::get_blocks_metadata() doesn't really need it either 🤣

The intention behind this PR was to address the PHP warning

Warning: Declaration of WP_Theme_JSON_6_1::get_style_nodes($theme_json) should be compatible with WP_Theme_JSON_5_9::get_style_nodes($theme_json, $selectors = Array) in /var/www/html/wp-content/plugins/gutenberg/lib/compat/wordpress-6.1/class-wp-theme-json-6-1.php on line 124

So we added $selectors = array() to the get_style_nodes() function arguments to make it compatible with the 5.9 method.

Then the linter started complaining that $selectors was never used in get_style_nodes() 😄

So passing $selectors down from get_style_nodes() to static::get_blocks_metadata() was a quick way to end the complaints and warnings.

What do you think a valid path to deprecate the argument might be?

I was thinking in a new PR we could:

  1. revert this PR
  2. remove get_style_nodes() from WP_Theme_JSON_5_9
  3. migrate any methods in WP_Theme_JSON_5_9 that use get_style_nodes() to WP_Theme_JSON_6_1 and remove the second argument
  4. Make sure we port these changes to WordPress 6.1 when that comes around 😄

Here's a PR that does all that:

@oandregal
Copy link
Member

Hey, while working on #46579 I realized this PR was not backported to WordPress 6.1. When we land that PR and then backport to WordPress 6.2 everything will be fine, but I wanted to raise in case you think this should be ported for a potential WordPress 6.1.X release.

@aaronrobertshaw
Copy link
Contributor

It looks like the fact this PR wasn't backported is still causing some knock on effects.

For example, #62465 needs to update the get_block_nodes signature but this PR modified it without backporting that change. When backporting #62465, I'll update the get_block_nodes function to match Gutenberg but it would be great if anyone that worked on this could double check those changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants