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

Site Logo Block: problems with Core/Plugin block loading #33177

Open
creativecoder opened this issue Jul 2, 2021 · 6 comments
Open

Site Logo Block: problems with Core/Plugin block loading #33177

creativecoder opened this issue Jul 2, 2021 · 6 comments
Labels
[Block] Site Logo Affects the Site Logo Block [Type] Bug An existing feature does not function as intended

Comments

@creativecoder
Copy link
Contributor

Description

Issues with loading the Site Logo block

Gutenberg plugin with WP 5.7.2

The hooks that update the site_logo option from the custom_logo theme mod, when the logo is updated in the Customizer, do not run. The result is that the logo cannot be updated from the Customizer.

I believe this is happening because _sync_custom_logo_to_site_logo is added via the setup_theme action, which has already been executed when the site-logo.php file is included, so the hooking into setup_theme has no effect at that point.

Gutenberg plugin with WP 5.8 RC

Both the Core php file and the plugin php file for the block are included. This results in all hooked functions being registered twice. I haven't seen any bugs with this yet, but I'm sure it could lead to unexpected results.

Step-by-step reproduction instructions

  • Run Gutenberg on a site with WP 5.7.2
  • Insert the Site Logo block into a post, set a logo image, and save
  • Go to the Customizer and update the logo to a different image.

Expected behaviour

The logo image set in the Customizer shows as the site logo

Actual behaviour

The previous logo is displayed.

WordPress information

  • WordPress version: 5.7.2
  • Gutenberg version: 11.0.0-rc.1
  • Are all plugins except Gutenberg deactivated? Yes
  • Are you using a default theme (e.g. Twenty Twenty-One)? Yes

Device information

  • Device: Desktop
  • Operating system: macOS
  • Browser: Firefox
@creativecoder creativecoder added [Type] Bug An existing feature does not function as intended [Priority] High Used to indicate top priority items that need quick attention [Block] Site Logo Affects the Site Logo Block labels Jul 2, 2021
@kathrynwp
Copy link

@creativecoder As it looks like this issue was fixed in this PR I'm going to go ahead and close this out, but if you're still experiencing the original issue, feel free to add a comment with additional details and I’ll be glad to reopen it.

@creativecoder
Copy link
Contributor Author

Hi @kathrynwp, thanks for taking a look. This is actually a different issue than what that PR addressed.

The fundamental problem has to do with

  • Gutenberg blocks that register functions on hooks that run before init (they will never be run b/c the files with those functions are included on the init hook).
  • Gutenberg blocks that duplicate/override hooks that are now in Core blocks, but don't unregister the Core versions.

For the Site logo block, now that the Core and Gutenberg versions are more in sync, there's probably no noticeable effect of either of those problems. But they are both lurking beneath the surface waiting to cause bugs when running the Gutenberg plugin.

@kathrynwp kathrynwp reopened this Aug 30, 2022
@kathrynwp
Copy link

Thanks for the clarification, I've reopened this!

@annezazu
Copy link
Contributor

👋🏼 Hey folks. I'm doing a sweep of high priority labeled issues to ensure the label remains actionable and relevant. As a result, I've removed the label from this issue because the work here has stalled and it's not clear whether this problem remains. In order to add the label back, @creativecoder can you provide a current status and confirm whether you believe it needs to be high priority so we can mobilize efforts? Ideally, anything with that label is getting actively worked on and moved forward.

@annezazu annezazu removed the [Priority] High Used to indicate top priority items that need quick attention label Dec 19, 2023
@creativecoder
Copy link
Contributor Author

Thank @annezazu ! It appears the underlying problem is still there, as I don't see any changes to how site-logo/index.php is loaded in Gutenberg.

However, this is no longer high priority, as the site-logo block php file is duplicated in Core, where it is loaded earlier (before the setup_theme hook) and so doesn't have the same problem with loading/hook order.

So this is now more a latent issue in Gutenberg that will reveal itself if

  • Any changes need to be made to _delete_site_logo_on_remove_custom_logo_on_setup_theme function in Gutenberg (right now, this function never runs in Gutenberg because it is added to the setup_theme hook after that hook has already run).
  • Any other blocks need to hook functions that run before init, because the block php files are not included until the init hook, so those functions won't run.

@annezazu
Copy link
Contributor

Thank you so much for this detailed recap and for helping confirm the label removal 🙌🏼

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Site Logo Affects the Site Logo Block [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

No branches or pull requests

3 participants