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.

Sign up for GitHub

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

Jump to bottom

fix: Undefined index "colors" warning #2982

Merged
merged 2 commits into from
Oct 11, 2017
Merged

fix: Undefined index "colors" warning #2982

merged 2 commits into from
Oct 11, 2017

Conversation

ayoolatj
Copy link
Contributor

Check that key exists and is not empty.

resolves: #2198

gziolo
gziolo requested changes Oct 11, 2017
View reviewed changes
Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

Thanks for opening this PR. It fixes the bug described in #2198:

<b>Notice</b>:  Undefined index: colors in <b>/srv/www/editor/htdocs/wp-content/plugins/gutenberg/lib/client-assets.php</b> on line <b>654</b><br />

but there is still another PHP notice in this code that needs to be fixed:

<b>Notice</b>:  Undefined index: wide-images in <b>/srv/www/wordpress-default/public_html/wp-content/plugins/lib/client-assets.php</b> on line <b>791</b><br>

@ayoolatj
Copy link
Contributor Author

I've updated my code to fix the second php notice

gziolo
gziolo reviewed Oct 11, 2017
View reviewed changes
lib/client-assets.php Outdated
@@ -783,12 +783,12 @@ function gutenberg_editor_scripts_and_styles( $hook ) {
$gutenberg_theme_support = get_theme_support( 'gutenberg' );
$color_palette = gutenberg_color_palette();

if ( $gutenberg_theme_support && $gutenberg_theme_support[0]['colors'] ) {
if ( $gutenberg_theme_support && ! empty( $gutenberg_theme_support[0]['colors'] ) ) {
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering if empty isn't powerful enough to make it possible to simplify it to:

if ( ! empty( $gutenberg_theme_support[0]['colors'] ) ) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I'm a little lost. Can you explain further?

Copy link
Member

@gziolo gziolo Oct 11, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Determine whether a variable is considered to be empty. A variable is considered empty if it does not exist or if its value equals FALSE. empty() does not generate a warning if the variable does not exist.

You don't need to check if $gutenberg_theme_support is truthy, because empty does that, too. It makes this line shorter and works exactly the same.

Copy link
Member

@gziolo gziolo Oct 11, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I just tested with the following code:

function mytheme_setup_theme_supported_features() {
	add_theme_support( 'gutenberg', null );
}

add_action( 'after_setup_theme', 'mytheme_setup_theme_supported_features' );

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that's right, should I update the PR with this?

Copy link
Member

Choose a reason for hiding this comment

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

If you like it, then go for it. Totally up to you :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated :)

@gziolo
Copy link
Member

gziolo commented Oct 11, 2017

That was quick, thanks. Testing your changes :)

gziolo
gziolo reviewed Oct 11, 2017
View reviewed changes
lib/client-assets.php Outdated
$color_palette = $gutenberg_theme_support[0]['colors'];
}

$editor_settings = array(
'wideImages' => $gutenberg_theme_support ? $gutenberg_theme_support[0]['wide-images'] : false,
'wideImages' => ( $gutenberg_theme_support && ! empty( $gutenberg_theme_support[0]['wide-images'] ) ) ? $gutenberg_theme_support[0]['wide-images'] : false,
Copy link
Member

Choose a reason for hiding this comment

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

In this place it would get simplified to the following:

'wideImages' => ! empty( $gutenberg_theme_support[0]['wide-images'] ) ),

@gziolo gziolo mentioned this pull request Oct 11, 2017
Closed
gziolo
gziolo approved these changes Oct 11, 2017
View reviewed changes
Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

I tested with the latest changes. I no longer see PHP notices. Nicely done @ayoolatj. I will leave it open to let you respond to my feedback. Let me know if you want to try to save some keystrokes as I suggested :)

@gziolo gziolo merged commit 1cea1fb into WordPress:master Oct 11, 2017
@gziolo
Copy link
Member

gziolo commented Oct 11, 2017

Thanks for another update. It looks great now. 🎉

DevinWalker added a commit to DevinWalker/gutenberg that referenced this pull request Oct 12, 2017
@DevinWalker
Merge branch 'master' into update/html-block-description
ea50751 
* master: (45 commits)
  Parser: Hide the core namespace in serialised data (WordPress#2950)
  fix: Undefined index warning (WordPress#2982)
  navigation via keydown
  PHPCS improvements (WordPress#2914)
  Update/html block description (WordPress#2917)
  Framework: Merge EditorSettingsProvider into EditorProvider
  Framework: Move the store initialization to a dedicated component
  Rotate header ellipsis to match block ellipsis
  add/459: Added support for ins, sub and sup
  Refresh nonce with heartbeat (WordPress#2790)
  Paste: add table support
  Bump version to 1.4.0. (WordPress#2954)
  Blocks: The custom classname should be persisted properly in the block's comment
  Editor: Fix the scroll position when reordering blocks
  Polish spacing in block ellipsis menu (WordPress#2955)
  Editor: HTML Editor per block (WordPress#2797)
  Show most frequently used blocks next to inserter (WordPress#2877)
  add/459: Added light grey background to selected inline boundaries
  Extend inline boundaries to other elements
  Move markdown fix to own plugin-compatibility file
  ...
DevinWalker added a commit to DevinWalker/gutenberg that referenced this pull request Oct 12, 2017
@DevinWalker
Merge branch 'master' into update/class-editor-block-description
79a29f6 
* master: (45 commits)
  Parser: Hide the core namespace in serialised data (WordPress#2950)
  fix: Undefined index warning (WordPress#2982)
  navigation via keydown
  PHPCS improvements (WordPress#2914)
  Update/html block description (WordPress#2917)
  Framework: Merge EditorSettingsProvider into EditorProvider
  Framework: Move the store initialization to a dedicated component
  Rotate header ellipsis to match block ellipsis
  add/459: Added support for ins, sub and sup
  Refresh nonce with heartbeat (WordPress#2790)
  Paste: add table support
  Bump version to 1.4.0. (WordPress#2954)
  Blocks: The custom classname should be persisted properly in the block's comment
  Editor: Fix the scroll position when reordering blocks
  Polish spacing in block ellipsis menu (WordPress#2955)
  Editor: HTML Editor per block (WordPress#2797)
  Show most frequently used blocks next to inserter (WordPress#2877)
  add/459: Added light grey background to selected inline boundaries
  Extend inline boundaries to other elements
  Move markdown fix to own plugin-compatibility file
  ...
DevinWalker added a commit to DevinWalker/gutenberg that referenced this pull request Oct 12, 2017
@DevinWalker
Merge branch 'master' into update/embed-block-description
1bf7e5b 
* master: (45 commits)
  Parser: Hide the core namespace in serialised data (WordPress#2950)
  fix: Undefined index warning (WordPress#2982)
  navigation via keydown
  PHPCS improvements (WordPress#2914)
  Update/html block description (WordPress#2917)
  Framework: Merge EditorSettingsProvider into EditorProvider
  Framework: Move the store initialization to a dedicated component
  Rotate header ellipsis to match block ellipsis
  add/459: Added support for ins, sub and sup
  Refresh nonce with heartbeat (WordPress#2790)
  Paste: add table support
  Bump version to 1.4.0. (WordPress#2954)
  Blocks: The custom classname should be persisted properly in the block's comment
  Editor: Fix the scroll position when reordering blocks
  Polish spacing in block ellipsis menu (WordPress#2955)
  Editor: HTML Editor per block (WordPress#2797)
  Show most frequently used blocks next to inserter (WordPress#2877)
  add/459: Added light grey background to selected inline boundaries
  Extend inline boundaries to other elements
  Move markdown fix to own plugin-compatibility file
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gziolo gziolo gziolo approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Theme Options: Undefined index "colors" when not defined in theme supports
2 participants
@ayoolatj @gziolo