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

Better error message when theme.json styles use a duotone preset not in settings #50714

Merged
merged 4 commits into from
May 19, 2023

Conversation

ajlende
Copy link
Contributor

@ajlende ajlende commented May 17, 2023

What?

Provides a more helpful error message when you've misspelled a duotone variable name or tried to use a variable that we don't have the duotone information to generate an SVG.

Why?

More clear error messages are more helpful for theme developers. And it's better to show a clear error than silently not render the filter since this problem exists within a hand-coded theme.json that a developer would be creating. Using the global styles UI shouldn't create this error because only presets are able to be selected from in the interface.

How?

Triggers a custom error message when the CSS var being used in styles isn't part of the presets defined in settings.

Testing Instructions

  1. Update theme.json styles.blocks['core/image'].filter.duotone = var(--wp--preset--duotone--does-not-exist).
  2. Add image block in the post editor.
  3. See the new error message.

Testing Instructions for Keyboard

Screenshots or screencast

Before

Notice: Undefined index: wp-duotone-does-not-exist in /Users/ajlende/Documents/gutenberg/lib/class-wp-duotone-gutenberg.php on line 715

After

Notice: Function WP_Duotone_Gutenberg::enqueue_global_styles_preset was called <strong>incorrectly</strong>. The duotone id "wp-duotone-prmary" is not registered in theme.json settings Please see <a href="https://wordpress.org/documentation/article/debugging-in-wordpress/">Debugging in WordPress</a> for more information. (This message was added in version 6.3.0.) in /Users/ajlende/Documents/wordpress-develop/src/wp-includes/functions.php on line 5865

Previously, in this PR using trigger_error.

Notice: The duotone id "wp-duotone-does-not-exist" is not registered in theme.json settings in /Users/ajlende/Documents/gutenberg/lib/class-wp-duotone-gutenberg.php on line 718

@ajlende ajlende added the [Type] Bug An existing feature does not function as intended label May 17, 2023
@ajlende ajlende self-assigned this May 17, 2023
@ajlende ajlende changed the title Fix unknown index error when theme.json styles use a duotone preset not in settings Better error message when theme.json styles use a duotone preset not in settings May 17, 2023
@ajlende ajlende force-pushed the fix/duotone-preset-error-unknown-index branch from 9e6c942 to ee8d435 Compare May 17, 2023 18:26
Copy link
Contributor

@scruffian scruffian left a comment

Choose a reason for hiding this comment

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

LGTM

@scruffian scruffian enabled auto-merge (squash) May 19, 2023 12:30
__( 'The duotone id "%s" is not registered in theme.json settings', 'gutenberg' ),
$filter_id
);
trigger_error( $error_message );
Copy link
Member

Choose a reason for hiding this comment

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

Trigger error? Let's use _doing_it_wrong instead.

Copy link
Contributor Author

@ajlende ajlende May 19, 2023

Choose a reason for hiding this comment

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

I was under the impression that _doing_it_wrong was used for developers calling functions incorrectly. The function is being called correctly, but the theme.json it's reading from has bad data. Is it still the right choice in this case?

EDIT: For reference, this is what I was basing things off of which uses trigger_error. https://github.com/WordPress/gutenberg/blob/trunk/lib/class-wp-theme-json-resolver-gutenberg.php#L501

@ajlende
Copy link
Contributor Author

ajlende commented May 19, 2023

I pushed up the change to use _doing_it_wrong, but I think that error message is worse. It now reads:

Notice: Function WP_Duotone_Gutenberg::enqueue_global_styles_preset was called <strong>incorrectly</strong>. The duotone id "wp-duotone-prmary" is not registered in theme.json settings Please see <a href="https://wordpress.org/documentation/article/debugging-in-wordpress/">Debugging in WordPress</a> for more information. (This message was added in version 6.3.0.) in /Users/ajlende/Documents/wordpress-develop/src/wp-includes/functions.php on line 5865

@scruffian scruffian merged commit 5584713 into trunk May 19, 2023
@scruffian scruffian deleted the fix/duotone-preset-error-unknown-index branch May 19, 2023 20:03
@github-actions github-actions bot added this to the Gutenberg 15.9 milestone May 19, 2023
@github-actions
Copy link

Flaky tests detected in ef60cd5.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5027636815
📝 Reported issues:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Style Engine /packages/style-engine [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