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

Add check theme support is an array before indexing #23104

Merged
merged 6 commits into from
Jun 12, 2020

Conversation

mkaz
Copy link
Member

@mkaz mkaz commented Jun 11, 2020

Description

Adds checks for return value from get_theme_support to validate it returns an array of at least size 1 that can be indexed into.

The get_theme_support may return false, and attempting to indexing causes a PHP notice.

Fixes #23085

How has this been tested?

Use a theme that does not support Global Styles, I had a Twenty Nineteen theme, but may not have been the most up-to-date.

Turn on PHP notice warnings as visible.

Load editor. Confirm warning does not show after fix.

Types of changes

Adds a couple of additional checks of the value returned from get_theme_support()

@mkaz mkaz added the Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json label Jun 11, 2020
@mkaz mkaz requested a review from oandregal June 11, 2020 20:17
@github-actions
Copy link

github-actions bot commented Jun 11, 2020

Size Change: +113 B (0%)

Total Size: 1.13 MB

Filename Size Change
build/block-editor/index.js 106 kB +25 B (0%)
build/block-editor/style-rtl.css 12.1 kB +66 B (0%)
build/block-editor/style.css 12.1 kB +67 B (0%)
build/block-library/editor-rtl.css 7.88 kB +1 B
build/block-library/index.js 129 kB -60 B (0%)
build/blocks/index.js 48.1 kB +3 B (0%)
build/components/index.js 195 kB +4 B (0%)
build/edit-widgets/index.js 9.33 kB -2 B (0%)
build/editor/index.js 44.8 kB +9 B (0%)
build/format-library/index.js 7.72 kB -1 B
build/plugins/index.js 2.56 kB +1 B
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/annotations/index.js 3.62 kB 0 B
build/api-fetch/index.js 3.4 kB 0 B
build/autop/index.js 2.83 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 7.22 kB 0 B
build/block-directory/style-rtl.css 892 B 0 B
build/block-directory/style.css 892 B 0 B
build/block-library/editor.css 7.89 kB 0 B
build/block-library/style-rtl.css 7.96 kB 0 B
build/block-library/style.css 7.96 kB 0 B
build/block-library/theme-rtl.css 684 B 0 B
build/block-library/theme.css 686 B 0 B
build/block-serialization-default-parser/index.js 1.88 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/components/style-rtl.css 19.5 kB 0 B
build/components/style.css 19.5 kB 0 B
build/compose/index.js 9.31 kB 0 B
build/core-data/index.js 11.4 kB 0 B
build/data-controls/index.js 1.29 kB 0 B
build/data/index.js 8.44 kB 0 B
build/date/index.js 5.47 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 568 B 0 B
build/dom/index.js 3.17 kB 0 B
build/edit-navigation/index.js 8.26 kB 0 B
build/edit-navigation/style-rtl.css 975 B 0 B
build/edit-navigation/style.css 974 B 0 B
build/edit-post/index.js 303 kB 0 B
build/edit-post/style-rtl.css 5.6 kB 0 B
build/edit-post/style.css 5.6 kB 0 B
build/edit-site/index.js 16.6 kB 0 B
build/edit-site/style-rtl.css 2.96 kB 0 B
build/edit-site/style.css 2.96 kB 0 B
build/edit-widgets/style-rtl.css 2.4 kB 0 B
build/edit-widgets/style.css 2.4 kB 0 B
build/editor/editor-styles-rtl.css 423 B 0 B
build/editor/editor-styles.css 423 B 0 B
build/editor/style-rtl.css 4.26 kB 0 B
build/editor/style.css 4.27 kB 0 B
build/element/index.js 4.64 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/style-rtl.css 502 B 0 B
build/format-library/style.css 502 B 0 B
build/hooks/index.js 2.13 kB 0 B
build/html-entities/index.js 622 B 0 B
build/i18n/index.js 3.56 kB 0 B
build/is-shallow-equal/index.js 710 B 0 B
build/keyboard-shortcuts/index.js 2.51 kB 0 B
build/keycodes/index.js 1.94 kB 0 B
build/list-reusable-blocks/index.js 3.12 kB 0 B
build/list-reusable-blocks/style-rtl.css 226 B 0 B
build/list-reusable-blocks/style.css 226 B 0 B
build/media-utils/index.js 5.29 kB 0 B
build/notices/index.js 1.79 kB 0 B
build/nux/index.js 3.4 kB 0 B
build/nux/style-rtl.css 616 B 0 B
build/nux/style.css 613 B 0 B
build/primitives/index.js 1.5 kB 0 B
build/priority-queue/index.js 789 B 0 B
build/redux-routine/index.js 2.85 kB 0 B
build/rich-text/index.js 14.8 kB 0 B
build/server-side-render/index.js 2.68 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.28 kB 0 B
build/url/index.js 4.06 kB 0 B
build/viewport/index.js 1.85 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

@oandregal oandregal added the [Type] Bug An existing feature does not function as intended label Jun 12, 2020
@oandregal
Copy link
Member

There's the gutenberg_experimental_get function that Jorge created recently. I was thinking that we could perhaps modify it in such a way that it does type checking and if the first parameter is not an array it returns a void array. Then we can do gutenberg_experimental_get( get_theme_support( 'editor-color-palette' ), '0' ) and avoid the nesting levels type checking requires for this code. We're going to do more of checking theme support for other things so this would be very useful.

@mkaz
Copy link
Member Author

mkaz commented Jun 12, 2020

I updated the PR with the change suggested, the one thing I don't really like is the check gets added into the function, when technically it shouldn't need it. The function should be able to assume it was called correctly, but it does simplify things.

lib/utils.php Outdated Show resolved Hide resolved
@mkaz mkaz merged commit 830651a into master Jun 12, 2020
@mkaz mkaz deleted the fix/23085-gs-theme-supports branch June 12, 2020 23:25
@github-actions github-actions bot added this to the Gutenberg 8.4 milestone Jun 12, 2020
@ellatrix ellatrix mentioned this pull request Jun 16, 2020
12 tasks
This was referenced Jun 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PHP notice in global-styles.php
2 participants