Skip to content

Remove PHP Notice when there is no logo an we are looking for an logo ID#14924

Merged
leogermani merged 2 commits intomasterfrom
fix/issue-14747
Mar 18, 2020
Merged

Remove PHP Notice when there is no logo an we are looking for an logo ID#14924
leogermani merged 2 commits intomasterfrom
fix/issue-14747

Conversation

@enejb
Copy link
Copy Markdown
Member

@enejb enejb commented Mar 9, 2020

Fixes #14747

Changes proposed in this Pull Request:

  • Removes a php notice when we are looking for a logo id but no logo id is set yet.

Testing instructions:

  • Create a new site or a site where no logo has been set yet.
  • Make sure you have debugging enabled.
  • Notice that you don't see a php error when you visit the customizer.

Proposed changelog entry for your changes:

  • Removes the PHP notice on the customizer page.

@enejb enejb added Bug When a feature is broken and / or not performing as intended [Feature] Site Icon [Status] Needs Review This PR is ready for review. labels Mar 9, 2020
@enejb enejb added this to the 8.4 milestone Mar 9, 2020
@enejb enejb requested a review from a team March 9, 2020 08:40
@enejb enejb self-assigned this Mar 9, 2020
@matticbot
Copy link
Copy Markdown
Contributor

matticbot commented Mar 9, 2020

Caution: This PR has changes that must be merged to WordPress.com
Hello enejb! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D40052-code before merging this PR. Thank you!

@jetpackbot
Copy link
Copy Markdown
Collaborator

jetpackbot commented Mar 9, 2020 •
edited
Loading

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Scheduled Jetpack release: April 7, 2020.
Scheduled code freeze: March 31, 2020

Generated by 🚫 dangerJS against a023193

kraftbj
kraftbj previously requested changes Mar 9, 2020
View reviewed changes
modules/theme-tools/site-logo/inc/functions.php Outdated
$logo = site_logo()->logo;
$logo_id = get_theme_mod( 'custom_logo' ); // Check for WP 4.5 Site Logo
$logo_id = $logo_id ? $logo_id : $logo['id']; // Use WP Core logo if present, otherwise use Jetpack's.
$logo_id = $logo_id ? $logo_id : ( isset( $logo['id'] ) ? $logo['id'] : false ); // Use WP Core logo if present, otherwise use Jetpack's.
Copy link
Copy Markdown
Contributor

@kraftbj kraftbj Mar 9, 2020

Choose a reason for hiding this comment

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

Is this is false should we better handle the $html at line 123 instead of passing false to wp_get_attachment_image?

Copy link
Copy Markdown
Member Author

@enejb enejb Mar 11, 2020

Choose a reason for hiding this comment

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

The way I understand we never be passing false to wp_get_attachment_image.
Since it would be caught in ( ! jetpack_has_site_logo() ) check and we are not suing the $logo_id in that block.
And keep $html as '';

@kraftbj kraftbj added [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. and removed [Status] Needs Review This PR is ready for review. labels Mar 9, 2020
@enejb enejb added [Status] Needs Review This PR is ready for review. and removed [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. labels Mar 11, 2020
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 11, 2020

Howdy! The Jetpack team has disappeared for a few days to a secret island lair to concoct new ways to make Jetpack one hundred billion percent better. As a result, your Pull Request may not be reviewed right away. Do not worry, we will be back next week to look at your work! Thank you for your understanding.

@enejb enejb requested a review from kraftbj March 12, 2020 09:42
leogermani
leogermani reviewed Mar 17, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@leogermani leogermani left a comment

Choose a reason for hiding this comment

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

I can't really reproduce the issue. I never see the NOTICE. Anyway the PR looks good.

Just one question:

Why do we check for jetpack_has_site_logo() and not for $logo_id here?

jetpack/modules/theme-tools/site-logo/inc/functions.php

Line 111 in 61d7383

if ( ! jetpack_has_site_logo() ) {

$logo_id is looking at get_theme_mod( 'custom_logo' ), while jetpack_has_site_logo() is looking for get_option( 'site_logo', null ).

@jeherve jeherve added [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. and removed [Status] Needs Review This PR is ready for review. labels Mar 17, 2020
@matticbot
Copy link
Copy Markdown
Contributor

matticbot commented Mar 17, 2020

enejb, Your synced wpcom patch D40052-code has been updated.

@zinigor
Copy link
Copy Markdown
Contributor

zinigor commented Mar 17, 2020

Sorry, I couldn't help myself. The logic in the function seemed very strange to me, so I decided to refactor it a bit, and pushed one more commit to this PR. Here's how you can test it.

  • First you need a site with a theme that doesn't support Core's Site Icon feature, like twentyten.
  • Next you need to enable Jetpack's Site logo by manually adding this snippet into your code that runs before Jetpack. I usually use a functions.php file inside wp-content/mu-plugins:
add_theme_support(
	'site-logo',
	array(
		'size' => 'thumbnail'
	)
);

add_action( 'wp_head', function() {
	jetpack_the_site_logo();
 });

Essentially this is a very crude way of adding a logo to your header. This also enables the Customizer entry inside the Site Identity tab.

  • After this is done, you can navigate to any site page and see the notice in your error log. If you already have set the logo picture you won't see the notice until you remove the option with this:
update_option('site_logo', null );

@zinigor
Copy link
Copy Markdown
Contributor

zinigor commented Mar 17, 2020

Oh, one more thing. @leogermani you were asking why we're checking jetpack_has_site_logo instead of Core's setting. The reason here is that this code only runs when Core's site icon feature is not available. If it is, we disable ours.

leogermani
leogermani approved these changes Mar 17, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@leogermani leogermani 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 the clarification @zinigor and for the extended testing instructions.

I can confirm the fix.

The logic is much more readable now as well!

@leogermani leogermani removed the [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. label Mar 17, 2020
@jeherve jeherve added the [Status] Ready to Merge Go ahead, you can push that green button! label Mar 18, 2020
jeherve
jeherve approved these changes Mar 18, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

This should be good to merge!

@jeherve jeherve dismissed kraftbj’s stale review March 18, 2020 10:45

changes addressed

@leogermani leogermani merged commit eb1ec6f into master Mar 18, 2020
@leogermani leogermani deleted the fix/issue-14747 branch March 18, 2020 12:46
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Mar 18, 2020
@jeherve
Copy link
Copy Markdown
Member

jeherve commented Mar 18, 2020

@enejb I'll let you handle the matching wpcom diff? D40052-code. Thank you!

jeherve added a commit that referenced this pull request Mar 20, 2020
@jeherve
Changelog: add #14924
aa83ec3
@kraftbj
Copy link
Copy Markdown
Contributor

kraftbj commented Mar 25, 2020

Committed it via r204773-wpcom to avoid it getting stale. cc: @enejb

jeherve added a commit that referenced this pull request Mar 31, 2020
@jeherve
8.4 Changelog (#15075)
0eb0b35 
* Initial changelog entry

* Changelog: add #14904

* Changelog: add #14910

* Changelog: add #14913

* Changelog: add #14916

* Changelog: add #14922

* Changelog: add #14924

* Changelog: add #14925

* Changelog: add #14928

* Changelog: add #14840

* Changelog: add #14841

* Changelog: add #14842

* Changelog: add #14826

* Changelog: add #14835

* Changelog: add #14859

* Changelog: add #14884

* Changelog: add #14888

* Changelog: add #14817

* Changelog: add #14814

* Changelog: add #14819

* Changelog;: add #14797

* Changelog: add #14798

* Changelog: add #14802

* Changelog: add #13676

* Changelog: add #13744

* Changelog: add #13777

* Changelog: add #14446

* Changelog: add #14739

* Changelog: add #14770

* Changelog: add #14784

* Changelog: add #14897

* Changelog: add #14898

* Changelog: add #14968

* Changelog: add #14985

* Changelog: add #15044

* Changelog: add #15052

* Update to remove Podcast since it remains in Beta

* Changelog: add #14803

* Changelog: add #15028

* Changelog: add #15065

* Changelog:add #14886

* Changelog: add #15118

* Changelog: add #14990

* Changelog: add #14528

* Changelog: add #15120

* Changelog: add #15126

* Changelog: add #15049

* Chanegelog: add #14852

* Changelog: add #15090

* Changelog: add #15138

* Changelog: add #15124

* Changelog:add #15055

* Changelog: add #15017

* Changelog: add #15109

* Changelog: add #15145

* Changelog:add #15096

* Changelog:add #15153

* Changelog: add #15133

* Changelog: add #14960

* Changelog: add #15127

* Changelog: add #15056

* Copy current changelog to changelog archive.

* Clarify changelog description
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jeherve jeherve jeherve approved these changes

@leogermani leogermani leogermani approved these changes

@kraftbj kraftbj Awaiting requested review from kraftbj

Assignees

@enejb enejb

Labels

Bug When a feature is broken and / or not performing as intended [Feature] Site Icon Touches WP.com Files

Projects

None yet

Milestone

8.4

Development

Successfully merging this pull request may close these issues.

Site Logo: Trying to access array offset on value of type null

7 participants

@enejb @matticbot @jetpackbot @zinigor @jeherve @kraftbj @leogermani