Make WordPress Core

Opened 8 months ago

Closed 8 months ago

Last modified 8 months ago

#59935 closed defect (bug) (fixed)

Site editor: logo

Reported by: priethor's profile priethor Owned by: hellofromtonya's profile hellofromTonya
Milestone: 6.4.2 Priority: normal
Severity: normal Version: 6.4
Component: Editor Keywords: has-testing-info has-unit-tests has-patch commit fixed-major dev-reviewed
Focuses: rest-api Cc:

Description

On the top left of the Site Editor, the W logo that serves as a back button to wp-admin should change to the Site Logo if the latter is set, as can be seen in WordPress 6.3.2. However, after upgrading to 6.4.1, the icon always shows the default W icon.

Steps to reproduce:

  1. Go to Appearance -> Editor
  2. Add a Site Logo block, upload an image to a header, and save it.
  3. The top-left W remains, although it should now be replaced by the image recently uploaded.

This is what I see in 6.4.1 (with or without Gutenberg plugin, it affects core):
https://i.ibb.co/vmq80kq/Sin-ti-tulo-4.png

This is what I should see, as seen in 6.3.2:
https://i.ibb.co/N3GYMsf/Sin-ti-tulo-3.png

Attachments (1)

SiteIcon-working-with-patch.gif (9.6 MB) - added by hellofromTonya 8 months ago.
Test Report: Site icon replacing W restored with PR 5691 ✅

Change History (23)

#1 @antonvlasenko
8 months ago

I'm working on this issue.

P.S. I can reproduce it.

The issue is clearly related to the REST API.

Last edited 8 months ago by antonvlasenko (previous) (diff)

This ticket was mentioned in PR #5691 on WordPress/wordpress-develop by @antonvlasenko.


8 months ago
#2

  • Keywords has-patch added; needs-patch removed

This PR aims to implement more granular control over which fields are included in the index response.

Trac ticket: https://core.trac.wordpress.org/ticket/59935

#3 @wildworks
8 months ago

Related to this, I found that a similar problem occurred with the Site Logo block.

https://github.com/WordPress/gutenberg/issues/56358

The patch currently in this ticket appears to be effective for the Site Logo block issue as well.

#4 follow-up: @NekoJonez
8 months ago

Shouldn't this be fixed in the Gutenberg repo? Since, I think this is a Gutenberg issue... As in, the version that's merged into core.

#5 @antonvlasenko
8 months ago

The issue that affected the Site Logo block, was introduced in https://core.trac.wordpress.org/ticket/57902, and resulted in the site_icon and site_icon_url fields being absent from the response, despite the site_icon_url field being included in the _fields GET parameter.
The PR I am proposing addresses this by ensuring that the site_icon* and site_logo fields are present in the response when they are explicitly requested in the _fields parameter.

Steps to test https://github.com/WordPress/wordpress-develop/pull/5691:

  1. Navigate to Appearance -> Editor.
  2. Add a Site Logo block, upload an image to to it, and save the page.
  3. The top-left W should be replaced with the image you just uploaded.

#7 in reply to: ↑ 4 @antonvlasenko
8 months ago

  • Focuses rest-api added

Replying to NekoJonez:

Shouldn't this be fixed in the Gutenberg repo? Since, I think this is a Gutenberg issue... As in, the version that's merged into core.

It doesn't appear to be Gutenberg related.
The issue lies in the WP_REST_Server class, which is part of the Core codebase.

#8 @hellofromTonya
8 months ago

  • Keywords changes-requested added
  • Milestone changed from Awaiting Review to 6.4.2
  • Owner set to hellofromTonya
  • Status changed from new to accepted

Moving into 6.4.2. Requested changes on the PR.

@hellofromTonya commented on PR #5691:


8 months ago
#9

@anton-vlasenko Hope you don't mind, but I did some work on the tests including simplifying them, relocating to group with the other "site_icon" test, and adding an unhappy path for that existing "site_icon" test. Can you do a confidence check on the latest version please?

@antonvlasenko commented on PR #5691:


8 months ago
#10

@anton-vlasenko Hope you don't mind, but I did some work on the tests including simplifying them, relocating to group with the other "site_icon" test, and adding an unhappy path for that existing "site_icon" test. Can you do a confidence check on the latest version please?

I absolutely don't mind. I find these changes beneficial, and they look good to me. Thank you, @hellofromtonya.
P.S. I've renamed $not_expected_field to $unexpected_field to align its name with $unexpected_fields.

#11 @hellofromTonya
8 months ago

  • Keywords has-unit-tests needs-testing added; changes-requested removed

Patch: https://github.com/WordPress/wordpress-develop/pull/5691

The patch is now ready for final testing / test reports. The source code changed slightly though its functionality remained the same. A new round of testing can help to validate its ready for commit.

#12 @ironprogrammer
8 months ago

LGTM! 👍🏻 Thanks for the PR!

Test Report

Patch tested: https://github.com/WordPress/wordpress-develop/pull/5691

Environment

  • Hardware: MacBook Pro Apple M1 Pro
  • OS: macOS 13.6.2
  • Browser: Safari 17.1, Google Chrome 119.0.6045.199
  • Server: nginx/1.25.3
  • PHP: 8.3.0
  • WordPress: 6.5-alpha-56966-src
  • Theme: twentytwentyfour v1.0

Actual Results

  • ✅ The default "W" icon is replaced with the newly set site icon after the page is refreshed.
  • ✅ If the site icon has alternatively been set via "Site Icon settings" (/wp-admin/customize.php?autofocus[section]=title_tagline) or a previous (classic theme) Customizer "Site Identity" update, re-enabling the "Use as site icon" on the Site Icon block will replace the previously set icon with the new icon after the page is refreshed.

Additional Notes

  • In test step 2, make sure that the Site Icon block's "Use as site icon" setting is enabled.
  • A page refresh (or navigation out and back into the site editor) is required to display the updated site icon.

@hellofromTonya
8 months ago

Test Report: Site icon replacing W restored with PR 5691 ✅

#13 @hellofromTonya
8 months ago

  • Keywords commit added; needs-testing removed

Patch: https://github.com/WordPress/wordpress-develop/pull/5691

Ready for commit. Will prep shortly.

#14 @hellofromTonya
8 months ago

Also confirmed the PR works when:

  • Resetting the Site Icon ✅
  • Replacing a site icon with a different image ✅

#15 @hellofromTonya
8 months ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 57147:

REST API: Restore site logo and icon in index.

Restores setting the site's logo, icon, and wp-admin's back button image (which defaults to W).

Prior to [56566], the site logo and icon were unconditionally added to the index. [56566] changed this by conditionally adding them if either the _links or _embedded fields were included. However, these fields are not included when using the Site Logo block, as it uses the site_logo, site_icon, and site_icon_url fields instead.

This changeset restores the functionality by checking specifically for the site_* fields when neither of the _links or _embedded fields are present.

Follow up to [56566].

Props antonvlasenko, hellofromTonya, ironprogrammer, priethor, wildworks.
Fixes #59935.

#17 @hellofromTonya
8 months ago

  • Keywords dev-feedback added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for 2nd committer sign-off to backport [57147] to the 6.4 branch.

#18 @hellofromTonya
8 months ago

  • Keywords fixed-major added

#19 @jorbin
8 months ago

  • Keywords dev-reviewed added; dev-feedback removed

[57147] looks good to for 6.4 branch.

#20 @hellofromTonya
8 months ago

Thanks @jorbin. Backporting now.

#21 @hellofromTonya
8 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 57154:

REST API: Restore site logo and icon in index.

Restores setting the site's logo, icon, and wp-admin's back button image (which defaults to W).

Prior to [56566], the site logo and icon were unconditionally added to the index. [56566] changed this by conditionally adding them if either the _links or _embedded fields were included. However, these fields are not included when using the Site Logo block, as it uses the site_logo, site_icon, and site_icon_url fields instead.

This changeset restores the functionality by checking specifically for the site_* fields when neither of the _links or _embedded fields are present.

Follow up to [56566].

Reviewed by jorbin.
Merges [57147] to the 6.4 branch.

Props antonvlasenko, hellofromTonya, ironprogrammer, priethor, wildworks.
Fixes #59935.

This ticket was mentioned in Slack in #core by jorbin. View the logs.


8 months ago

Note: See TracTickets for help on using tickets.