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

[enejb]

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.

[matticbot]

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]

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]

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

[enejb]

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 '';

[github-actions]

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.

[leogermani]

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 ).

[matticbot]

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

[zinigor]

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]

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]

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!

[jeherve]

This should be good to merge!

changes addressed

[jeherve]

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

[kraftbj]

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