Gallery block: turn on auto-migration of v1 Gallery blocks to v2 format when edited #36191
Conversation
glendaviesnz
added
[Status] In Progress
|
Size Change: -329 B (0%) Total Size: 1.1 MB
ℹ️ View Unchanged
|
noisysocks
added
the
Backport to WP 6.7 Beta/RC
glendaviesnz
force-pushed
the
update/gallery-block-disable-v2-when-useBalanceTags-set
branch
from
73bec70
to
3c2480c
Compare
glendaviesnz
removed
the
[Status] In Progress
glendaviesnz
requested review from
mkevins,
spacedmonkey and
TimothyBJacobs
as code owners
glendaviesnz
changed the title
|
Hi Glen 👋 I encountered an issue when testing this on mobile. Steps:
Preview of Gallery:
So, seemingly something is destroying the gallery on when the editor is opened the second time (maybe in the deprecations flow 🤔 ? ). |
There was a problem hiding this comment.
Nice work @glendaviesnz! This is working well for migrating from current v1 markup to v2, while preserving columns settings and "link to" settings 👍
I also tested each of the core__gallery__deprecated-x.html deprecations in the v1 version of the editor. Initially I thought I ran into an issue, but then realised I hadn't pulled in the latest changes, but as of e43a6a7 each of the deprecations is migrating correctly when switching between the code and visual editor views 👍
It's all working well for me in practice. Looking at the diff, the main issue I noticed is that a couple of the fixtures (shortcode-matching-out.html and wordpress-out.html) and a snapshot appear to be stripping the internal img element in the diff, but I wasn't quite sure how best to test them in practice, so not sure if it's an issue? Other than that, I left a couple of tiny nits, but feel free to ignore if they're not relevant!
| @@ -21,3 +21,11 @@ export const pickRelevantMediaFiles = ( image, sizeSlug = 'large' ) => { | |||
| } | |||
| return imageProps; | |||
| }; | |||
|
|
|||
| export function isGalleryV2Enabled() { | |||
| if ( typeof process !== 'undefined' && process.env?.NODE_ENV === 'test' ) { | |||
There was a problem hiding this comment.
This is possibly a bit nit-picky, and I imagine it doesn't matter too much since it's a simple check. But I was wondering if this needs to be in run-time code. Would it be possible to set window.wp.galleryBlockV2Enabled to true via globals in Jest config instead? I don't think it's a blocker, though, because the check is neatly tucked away inside the isGalleryV2Enabled function so would be easy to change later if we want to.
There was a problem hiding this comment.
🤦 yeh, jest globals is a much better option - completely overlooked that for some reason when trying to set this for test runs! Have moved there.
| @@ -97,9 +101,11 @@ class NativeEditorProvider extends Component { | |||
| galleryWithImageBlocks, | |||
| } = this.props; | |||
|
|
|||
| // TODO: remove this as unnecessary since we are setting in constructor? | |||
| window.wp.galleryBlockV2Enabled = galleryWithImageBlocks; | |||
There was a problem hiding this comment.
I haven't tested this, but it looks like it can be removed if it's set in the constructor?
There was a problem hiding this comment.
Thanks Andrew! That was a WIP "note to self" 😃 that I intended to circle back to and make sure I wan't missing anything about when this is set from the fetch response (originally this was setting a value in the store via updateSettings, which couldn't be done in the constructor - probably the reason it was set after mount initially). 👍
| return true; | ||
| } | ||
|
|
||
| return window.wp.galleryBlockV2Enabled; |
There was a problem hiding this comment.
Tiniest of tiny nits, but since this function is called isGalleryV2Enabled, should this return a boolean (e.g. !!)? Also, I assume that the wp object will always be available in practice, but I see that elsewhere in the Gallery block where window.wp is accessed, we've used optional chaining. Not sure if it's needed here, too?
There was a problem hiding this comment.
should this return a boolean
Hm.. window.wp.galleryBlockV2Enabled already is a boolean, I wonder, do you mean to coerce with !! for the case where it is undefined?
Not sure if it's needed here, too?
Good question.. I had contemplated that as well.. my thinking at the time (at least from the mobile perspective) is that window.wp should be defined (and as an Object), and that since we are altering user content on the basis of this flag, it'd be better to fail fast if that assumption is incorrect, to avoid incorrectly transforming the content. Otoh, this is function is shared cross-platform, so it'd be good to know from the web side whether this assumption is valid - and whether or not failing fast is appropriate there as well in case it's undefined. 🤔
Also, maybe it's actually fine on mobile and web to default to false(y) when the assumption is not met? 🤔 One of the hesitations I originally had was born out of the race conditions we've encountered while testing / debugging these flows - not sure whether the presence (or absence) of window.wp, if even possible, could also be a timing-based condition?
There was a problem hiding this comment.
@mkevins, @andrewserong I wonder if we want to default to v2 being enabled if window.wp.galleryBlockV2Enabled is not explicitly set to false, eg. return window.wp?.galleryBlockV2Enabled === false ? false : true; - the reason being that the disabled case with use_balanceTags === 1 and WP < 5.9 is the edge case so are we better to assume we use v2 if wp.galleryBlockV2Enabled was undefined for some reason?
There was a problem hiding this comment.
Thanks for the thoughts Matthew and Glen!
I wonder, do you mean to coerce with
!!for the case where it isundefined?
Yes, I was mostly thinking of how do we want to handle the case of undefined.
I wonder if we want to default to v2 being enabled if
window.wp.galleryBlockV2Enabledis not explicitly set to false
That logic sounds good to me, as you say, the presence of the flag is there to deal with an explicit edge case, so handling it with an explicit check for false sounds good to me 👍
glendaviesnz
force-pushed
the
update/gallery-block-disable-v2-when-useBalanceTags-set
branch
from
612a9e4
to
dd416ed
Compare
| export function isGalleryV2Enabled() { | ||
| // We want to fail early here, at least during beta testing phase, to ensure | ||
| // there aren't instances where undefined values cause false negatives. | ||
| if ( ! window.wp || typeof window.wp.galleryBlockV2Enabled !== 'boolean' ) { |
There was a problem hiding this comment.
Just wondering, if this PR is backported to the WP core beta, then will the logic in gutenberg_check_gallery_block_v2_compatibility also be backported? Without it, in 5.9, with Gutenberg deactivated, where wp.galleryBlockV2Enabled isn't set, will this throw an error?
In that case, it could be better to treat the v2 gallery block setting being undefined as true 🤔. I might be missing some context with the impact on mobile, though!
There was a problem hiding this comment.
Good question, Andrew. For the mobile perspective, I'm going to test making true default (because some users will not have a cached value when the editor is initialized):
diff --git a/packages/edit-post/src/editor.native.js b/packages/edit-post/src/editor.native.js
index b045724a04..7f71d7f68a 100644
--- a/packages/edit-post/src/editor.native.js
+++ b/packages/edit-post/src/editor.native.js
@@ -31,7 +31,9 @@ class Editor extends Component {
super( ...arguments );
// need to set this globally to avoid race with deprecations
- window.wp.galleryBlockV2Enabled = props.galleryWithImageBlocks;
+ // defaulting to true to avoid issues with a not-yet-cached value
+ const { galleryWithImageBlocks = true } = props;
+ window.wp.galleryBlockV2Enabled = galleryWithImageBlocks;
if ( props.initialHtmlModeEnabled && props.mode === 'visual' ) {
// enable html mode if the initial mode the parent wants it but we're not already in itFor mobile, we won't face the same backporting issue, since users on older versions won't encounter this logic, and users on newer versions will default to true (assuming all goes well with testing 👆 ). I wonder if something similar would work (or be appropriate) for web? Or perhaps that is an error that we'd like to surface in beta (if it exists)?
There was a problem hiding this comment.
Thanks Matthew, nice to see that's how we'll handle the situation in mobile 👍. Let's see what @glendaviesnz thinks for the web when he's back on tomorrow 🙂
There was a problem hiding this comment.
@andrewserong gutenberg_check_gallery_block_v2_compatibility will also need to be pulled into WP 5.9. The explicit error we are throwing here is mostly for test purposes to see if we ever hit that explicit exception during testing - I wasn't thinking we would leave that in in there permanently, but instead more likely default to true once we are comfortable there aren't any funny edge cases we have overlooked that would have that value as undefined when it really should be false - does that make sense?
There was a problem hiding this comment.
Yes, thanks for confirming @glendaviesnz! That does sound like a good way to handle it for the beta and so that issues don't go unnoticed.
It sounds like in that case we might want to open up a PR in wordpress-develop to add in the check_gallery_block_v2_compatibility function prior to this PR landing, since I think the PHP code gets merged in separately from the JS being released in npm packages for 5.9?
There was a problem hiding this comment.
@andrewserong check_gallery_block_v2_compatibility is in the gutenberg repo and added as part of this PR, so was thinking that it gets included in 5.9 when this PR merge is cherry picked? I don't see any of the other methods in lib/compat.php anywhere in the `wordpress-develop repo, am I missing something?
There was a problem hiding this comment.
@glendaviesnz good question — I'm not 100% sure of the order in which things go into 5.9. I believe there are two steps: commits for 5.9 are cherry picked into the wp/trunk branch and then new npm package releases are published (e.g. in this recent commit). Then, the next step is that over in wordpress-develop there's a PR created that updates trunk to point to the new packages (e.g. WordPress/wordpress-develop#1883).
It looks like that latter PR includes some PHP changes in addition to bumping the package numbers. I'm not sure if these changes needed to be made manually? So for the case of check_gallery_block_v2_compatibility, it'd be good to confirm where it'll be merged (or how best to do it) before we merge this PR.
CC: @noisysocks — just a question for the 5.9 release, this PR includes an additional function check_gallery_block_v2_compatibility that currently lives in lib/compat.php in Gutenberg, but will need to exist in core. Do we need to open up a separate PR in wordpress-develop for this function, or will it get rolled in when the PR is cherry picked into 5.9?
There was a problem hiding this comment.
When I prepare the Core patch that updates packages (e.g. WordPress/wordpress-develop#1883) every Monday, I do a diff of what PHP has changed in the Gutenberg plugin (git diff $old_sha wp/trunk **/*.php) and then manually make those changes in Core.
For big PHP changes, I appreciate if the PR author can open a ticket in Core Trac and make those changes themselves. That makes it less likely I have to stay up late on Monday 😀
For smaller changes such as this one, it's okay to leave it with me. You can help me (or any future release lead) out by leaving a note in the doc comment which describes how the change should look once added to Core.
There was a problem hiding this comment.
@glendaviesnz this PR is testing well for me, thanks for addressing all the global mock for galleryBlockV2Enabled, and it sounds like we're good to go in regards to merging in the PHP change 👍
I haven't been able to fault the changes in this PR, but when I went to test a v1 block with external images, I noticed that I'm unable to click any of the buttons in the v1 gallery block to replace an image with an external image. It's these Upload, Media Library, and Insert from URL buttons in the placeholder that don't appear to be clickable for me (only in the v1 version):
Are you able to replicate that? It appears to be unrelated to this PR, but meant that I wasn't too sure how to test that external images were migrated correctly. Other than that, this change LGTM!
I think I've worked out a fix for this @glendaviesnz, so I'll open up a PR soon 🙂 |
|
I've added a few more commits to fix some issues with the flag (default value, cached value, and fetch response): 0885b7a, 73f556b, 1b6df54. I've also performed a bunch of tests on a Jurassic Ninja site (WordPress 5.8.2) w/ the plugin built locally from this branch installed: Gallery Auto-conversion TestsTo test the auto-conversion implementation, I used a test site with ssh access, toggled the remote flag and the locally cached flag, and opened posts with existing V1 and V2 content. The table below shows the results of these tests:
In order to toggle the remote flag, the command in the WordPress directory is: In order to manipulate the local cache, the App Inspection tab can be used in Android Studio. Under the These tests have only been conducted for Android, though similar behavior should be expected for iOS, excepting that when the local cache is not populated, a default value of Although many cases in this table result in content loss, the proportion of rows in this table in no way reflects the proportion of users / scenarios where these issues will be encountered. Most of these scenarios are edge cases involving the user toggling the deprecated |
|
@mkevins Thank you for the mobile fixes and the detailed testing and instructions 🙇
I would agree with that. The content loss scenarios are edge cases and most of our users should not be affected. |
glendaviesnz
force-pushed
the
update/gallery-block-disable-v2-when-useBalanceTags-set
branch
from
bc7a1f2
to
da99acb
Compare
|
I've run through the test instructions and it works as described. ✅ Before checking out this PR, on a site on WP < 5.9 set use_balanceTags option to 1
Works on both the settings dashboard and CLI.
Although the CLI doesn't provide any upgrade feedback
I ran out of time to test more thoroughly, but will pick it up in the morning. |
|
Things are working well still for me! CLI error appears with the latest change 🌮
I noticed one peculiarity: When I have
I ran out of time to debug (again) sorry, but would we need to do something similar to what we did in #36388? |
|
|
||
| if ( 1 === (int) $new_value && version_compare( $wp_version, '5.9', '<' ) ) { | ||
| /* translators: %s: Minimum required version */ | ||
| $message = sprintf( __( 'Gutenberg requires WordPress %s or later in order to enable the “Correct invalidly nested XHTML automatically” option. Please upgrade WordPress before enabling.', 'gutenberg' ), '5.9' ); |
There was a problem hiding this comment.
Not a biggie, but would it possible to strip or replace the HTML entities for the CLI?
There was a problem hiding this comment.
The chance of somebody toggling on this option via CLI with gutenberg 12.1+ on WP < 5.9 is so slim as to not warrant the bother of adding a string replace method here in my view.
Nice catch! It looks like this is an existing bug - I get the same thing on 10.9.0, which is before the new Gallery was merged. It will take a bit of work to fix this as it requires for additional calls to getMedia after the transform in order to get the images |
…nabled and aren't on WP 5.9 or higher
… true in other instances
This makes sure the global value for the flag is established when the editor is initialized, since some users may not have the value cached locally yet. This is important to avoid incorrectly branching in the parsing step, resulting in an incompatible block format.
This adds the flag to the destructuring of the fetch callback (so it is not binding to the stale closure value), and also conditionally updates the global only if the type is explicitly boolean. This avoids a scenario in which updates to other editor settings which omit the gallery flag property result in the global value being overwritten with `undefined`.
This adds logic to ensure we report the gallery flag when the editor settings are fetched.
This allows the value to be set to default `true` on the JS side, instead of defaulting to a value of `false` from iOS native code.
glendaviesnz
force-pushed
the
update/gallery-block-disable-v2-when-useBalanceTags-set
branch
from
97ad0dd
to
c752d64
Compare
Oh great! Thanks for checking that it's an existing issue. In that case, it makes sense that we don't address it here and rely on the new gallery upgrade. Sounds good. |
There was a problem hiding this comment.
Great work again @glendaviesnz and @mkevins! This is testing well for me, and I haven't been able to find any faults with it 😀
✅ The auto-migration works correctly, irrespective of the use_balanceTags option when I'm running WP 5.9
✅ In 5.8.x, with use_balanceTags set to 1, editing the gallery remains in the v1 version of the gallery block
✅ In 5.8.x, if use_balanceTags is set to 0, I'm unable to set it to 1 and the error message is rendered in the UI and in the CLI, and the UI message looks good:
✅ In 5.8.x, the v1 gallery is being correctly auto-migrated to the v2 format, with settings and external images preserved correctly
✅ Updated text for the app version notice when inserting a gallery block reads well to me
This LGTM, and I did a quick visual skim of the code changes again. I'm not very familiar with React Native, but no issues jumped out at me from looking over the changes 🙂
There was a problem hiding this comment.
I went through the same testing steps as before in #36191 (comment)
I also added a bunch of v1 galleries to a page, some with external images. All migrated as expected.
The v2 gallery on its own works as expected after migration as well.
🥇 Nice work!
glendaviesnz
deleted the
update/gallery-block-disable-v2-when-useBalanceTags-set
branch
noisysocks
removed
the
Backport to WP 6.7 Beta/RC
…at when edited (#36191) * Remove update gallery modal * Add check for gallery v2 compat (block sites with use_BalanceTags option enabled and aren't on WP 5.9 or higher) to editor init so it is available when block deprecation pipeline runs * Remove references to gallery experimental flag * Add missing transformation updates * Update fixture to be compat with v2 of gallery block * Fix unit tests * Fix issue with link destination not being set when block migrated * Use `window.wp` global instead of store for gallery flag To resolve a race with the deprecations and asynchronous store updates for the gallery flag, we can use a property on the global wp object instead. * Simplify deprecations, etc. by always using using v1 methods if use_balanceTags is on regardless of content format * Move assignment of gallery global flag to native Editor. This moves the setting of the global flag higher in the view hierarchy, since the provider's constructor is still too late (parsing code invokes the deprecations before even the provider is instantiated). * Default to `true` for gallery flag when not yet cached This makes sure the global value for the flag is established when the editor is initialized, since some users may not have the value cached locally yet. This is important to avoid incorrectly branching in the parsing step, resulting in an incompatible block format. * Only update gallery flag if fetch result is explicitly boolean * Update the wording of the mobile warning * Add filter to prevent use_balanceTags being enabled on WP < 5.9 * Switch to using settings_error for use_balanceTags notice * Add a CLI error message Co-authored-by: Glen Davies <[email protected]> Co-authored-by: Matthew Kevins <[email protected]>
Description
Turns on automigration of v1 gallery blocks to v2 format unless a site has
use_BalanceTagsenabled and is not running WP >= 5.9Testing
use_balanceTagsback to 1 and you should get an error message about needing to upgrade to WP 5.9use_balanceTagssite, and v2 for othersImportant note: It is not possible to load v2 version blocks in a site with
use_balanceTagson and WP < 5.9, they will show an invalid error. The plan is to add some sort of explanatory error to indicate that user has to turn offuse_balanceTagsor upgrade to 5.9 in these instances - there is no point in showing a valid block as it will just break when the user saves it, but the user will not know unless they reload or view in the frontend, so better to fail on first editor load.You can use
wp option set use_balanceTags 1to toggle the option, or you can make the option available at/wp-admin/options-writing.phpby runningwp option set initial_db_version 32452Screenshots
v1 Gallery block:
v2 Gallery block:
