Site Logo: Update url for site icon settings with fallback for WP core versions earlier than 6.5 #59485
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…e versions earlier than 6.5
andrewserong
added
[Type] Bug
andrewserong
requested review from
fullofcaffeine
and removed request for
a team
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress. If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged. If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack. Thank you! ❤️ View changed files❔ lib/compat/wordpress-6.5/compat.php |
|
Size Change: +41 B (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
andrewserong
added
the
Backport to WP Beta/RC
ramonjd
approved these changes
Mar 1, 2024
There was a problem hiding this comment.
Thanks for getting this PR up @andrewserong
Looks like a less intrusive way to get this info to the editor.
Might be good to get other opinions too.
With 6.4
With 6.5
This makes me imagine the usefulness of an editor settings entry for admin urls, where we could store admin urls generated using get_admin_url(), e.g,
await wp.data.resolveSelect( 'core/block-editor' ).getSettings();
/*
{
...,
admin_urls: {
'options-general': `/wp-admin/options-general.php`,
'users': ...,
...
}
}
*/But I digress...
It would make this type of issue trivial and ensure that the admin urls went through admin_url filters.
lib/experimental/editor-settings.php
Outdated
| // Support the previous location for the Site Icon settings. To be removed | ||
| // when the required WP core version for Gutenberg is >= 6.5.0. | ||
| if ( ! is_wp_version_compatible( '6.5' ) ) { | ||
| wp_add_inline_script( 'wp-block-editor', 'window.__experimentalUseCustomizerSiteLogoUrl = true', 'before' ); |
There was a problem hiding this comment.
I'm wondering if this could have a better home in compat/6.5?
It's related to compatibility, and also it would be deleted as a matter of course when Gutenberg supports 6.5 and above.
There was a problem hiding this comment.
Yeah that's a good point, stuff in lib/experimental doesn't get audited for removal when we update the lowest supported WP.
There was a problem hiding this comment.
I'm wondering if this could have a better home in compat/6.5?
Good point. My main thinking in keeping it under lib/experimental was that compat/6.5 typically houses things that are intended to be backported, and it's probably a good thing for all the window.__experimental* globals to be co-located.
That said, I'm happy to move it around. Did you have a particular file in mind? It looks like most of the current ones are there to match files in wordpress-develop 🤔
There was a problem hiding this comment.
Oh wait, we have a compat/wordpress-6.5/compat.php file — I reckon that might be a good home, I can move it there 👍
There was a problem hiding this comment.
I think ideally it would be a new compat/wordpress-6.5/editor-settings.php file, because none of the existing files are related in any way 😅
I see compat folders as the place we put things we'll want to remove when we no longer support that version. It's not always stuff that's to be/has been backported, but anything that is version-specific. Really it's mostly for the sake of easier cleanup later!
There was a problem hiding this comment.
I think it's okay to add a new file, e.g., lib/compat/wordpress-6.4/theme-previews.php
Also maybe with a comment to say that it doesn't need backporting?
There was a problem hiding this comment.
compat.php already has this heading * WordPress 6.5 compatibility functions., so I think that might be a good home for now.
There was a problem hiding this comment.
Thanks for the discussion, folks! I've moved the location over in b8d9ae2
Let me know if you'd like any further changes! 🙂
| const shouldUseNewUrl = ! window?.__experimentalUseCustomizerSiteLogoUrl; | ||
|
|
||
| const siteIconSettingsUrl = shouldUseNewUrl | ||
| ? siteUrl + '/wp-admin/options-general.php' |
There was a problem hiding this comment.
Shame there's no id we can use as a hash value. The other options have them and are used in core, e.g., #home in esc_url( admin_url( 'options-general.php' ) . '#home' ).
wp-admin/options-general.php#choose-from-library-button is possible, but it's not great.
Which reminds me that Gutenberg should probably be using admin_url() or get_admin_url() for such links.
There was a problem hiding this comment.
Which reminds me that Gutenberg should probably be using admin_url() or get_admin_url() for such links.
Great point — that could be a good thing to look into for 6.6, since folks should be able to filter admin urls and have it propagate to links in Gutenberg 👍
|
Thanks for the quick review!
That does sound like a good idea. Is it worth opening an issue to keep track of it? It might be a good thing to dig into for a subsequent WP release at some stage, because I could imagine an issue like this one coming up again and requiring another similar quick fix 🤔 |
Yes! I'm happy to open an issue. I was just running my mouth mainly. 🙇🏻 |
But in the process you raised some very good points, thank you!! |
youknowriad
pushed a commit
that referenced
this pull request
Mar 4, 2024
…e versions earlier than 6.5 (#59485) * Site Logo: Update url for site icon settings with fallback for WP core versions earlier than 6.5 * Move PHP changes to compat.php file Co-authored-by: andrewserong <andrewserong@git.wordpress.org> Co-authored-by: ramonjd <ramonopoly@git.wordpress.org> Co-authored-by: tellthemachines <isabel_brison@git.wordpress.org> Co-authored-by: t-hamano <wildworks@git.wordpress.org> Co-authored-by: Mamaduka <mamaduka@git.wordpress.org> Co-authored-by: carolinan <poena@git.wordpress.org> Co-authored-by: richtabor <richtabor@git.wordpress.org> Co-authored-by: aaronjorbin <jorbin@git.wordpress.org>
|
I just cherry-picked this PR to the update/packages-6.5-rc1 branch to get it included in the next release: b60b97c |
youknowriad
added
Backported to WP Core
youknowriad
pushed a commit
that referenced
this pull request
Mar 4, 2024
…e versions earlier than 6.5 (#59485) * Site Logo: Update url for site icon settings with fallback for WP core versions earlier than 6.5 * Move PHP changes to compat.php file Co-authored-by: andrewserong <andrewserong@git.wordpress.org> Co-authored-by: ramonjd <ramonopoly@git.wordpress.org> Co-authored-by: tellthemachines <isabel_brison@git.wordpress.org> Co-authored-by: t-hamano <wildworks@git.wordpress.org> Co-authored-by: Mamaduka <mamaduka@git.wordpress.org> Co-authored-by: carolinan <poena@git.wordpress.org> Co-authored-by: richtabor <richtabor@git.wordpress.org> Co-authored-by: aaronjorbin <jorbin@git.wordpress.org>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Resolves #59382
Update the site icon settings url for the Site Logo block to point to the general settings page, now that WP core 6.5 includes the ability to update the site icon there. Do so while preserving the old URL on Gutenberg when running WP Core < 6.5.
Note: there were other options to this PR suggested over in #59382. I think this approach is likely one of the least disruptive, since it doesn't require any PHP changes in core for 6.5. I may not have time to iterate on this PR, so if anyone feels strongly about going with a different approach, feel free to close this one out!
Why?
For 6.5, it'd be good for the url to point to the general settings so that users aren't directed to the customizer. However, it's important to preserve Gutenberg compatibility with earlier WP core versions, until the required core version is at least 6.5.0.
How?
window.__experimentalUseCustomizerSiteLogoUrlvalue, that only exists if the current WP core version is not compatible with6.5.Testing Instructions
Use as Site Icontoggle should direct to the general settings page ofwp-admin.Testing Instructions for Keyboard
Screenshots or screencast
Running WP 6.5 Beta 3
The url should direct to the general options page in
wp-admin:Running WP 6.4.3
In this version, the url should still direct to the customizer: