Skip to content

Theme JSON: Fix notice when unknown elements are specified #3026

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

Closed
wants to merge 2 commits into from

Conversation

scruffian
Copy link

@scruffian scruffian commented Jul 26, 2022

If a theme.json file specifies an unknown element then we'll get a notice from line 1482 in this file. This adds an array_key_exists check before we get the value of the array at that key, to avoid the notice.

Gutenberg issue: WordPress/gutenberg#42649
Trac issue: https://core.trac.wordpress.org/ticket/56291


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.

@hz-tyfoon
Copy link

If I'm following correctly.
This PR attempting to fix this WordPress/gutenberg#42649.

@hz-tyfoon
Copy link

Hey @scruffian, 👋
I've just checked your codes in the PR commit: 93f9f19

Your code doesn't solve the gutenberg issue rather it contains another error.

Please Test the code U write before creating PR. Thanks. 🙂

'path' => array( 'styles', 'elements', $element ),
'selector' => static::ELEMENTS[ $element ],
);
if ( array_key_exists( $element, static::ELEMENTS ) ) {

Choose a reason for hiding this comment

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

hey @scruffian ,

This line containing the if check should be below the foreach check.
Currently the test is failing because the $element is undefined.

Kindly check please.

Choose a reason for hiding this comment

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

sorry about the typo.
I meant the foreach loop.

To be more clear:
line 1480 should be at line 1479
and
line 1479 should be at line 1480

Copy link
Author

Choose a reason for hiding this comment

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

Sorry about that

Copy link

@rudlinkon rudlinkon left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link

@hz-tyfoon hz-tyfoon left a comment

Choose a reason for hiding this comment

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

Looking good now. Thanks @scruffian

@scruffian
Copy link
Author

I made a patch for this on trac: https://core.trac.wordpress.org/ticket/56291

@hz-tyfoon
Copy link

hz-tyfoon commented Jul 27, 2022

May be adding the trac link in the 'description of this PR' will auto link this PR in the trac.
@scruffian

@scruffian
Copy link
Author

This was solved in a different way

@scruffian scruffian closed this Oct 5, 2022
@scruffian scruffian deleted the fix/element-notice branch October 5, 2022 09:00
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