user_update_8011 calls module_enable. That's as broken as it gets. If we were to call update_module_enable instead, are we going to copy the D7 schema to image_schema_0 just so that function image_update_8001 can drop them? Obviously we also need to copy the default config as well.

The other (trivial) issue that this needs to go into a helper function somewhere cos it might be that other modules also want to enable image.

CommentFileSizeAuthor
#20 enable-image-safely-1894328-19.patch4.37 KBberdir
#14 enable-image-safely-1894328-14.patch4.16 KBberdir
#14 enable-image-safely-1894328-14-interdiff.txt1.75 KBberdir
#12 enable-image-safely-1894328-12.patch3.86 KBberdir
#5 enable-image-safely-1894328-5.patch3.52 KBberdir
#5 enable-image-safely-1894328-5-interdiff.txt1.62 KBberdir
#4 enable-image-safely-1894328-4.patch2.77 KBberdir

Comments

catch’s picture

catch
he/him
Primary language English
commented
Priority: Major » Critical
Issue tags: +D8 upgrade path
berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Using update_module_enable() either requires us to rewrite it so that it adds the update functions to the current batch (changing batch to use the queue backend would make that rather easy I guess) or rewrite our upgrade tests.

chx’s picture

chx commented

Nah, just add image_enable_8002 which jumps image to 8002.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Active » Needs review
StatusFileSize
new enable-image-safely-1894328-4.patch2.77 KB

What about this?

- Prevent update_module_enable() from changing the module version it was only disabled
- Add an argument to set the schema version to a specific value instead of 0
- use update_module_enable() to install image.module with version 8002.
- Install default config and run image_install() in case the module was uninstalled before.

Probably needs some better test coverage to make sure that the default image styles and config exists.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
StatusFileSize
new enable-image-safely-1894328-5-interdiff.txt1.62 KB
new enable-image-safely-1894328-5.patch3.52 KB

Per discussion in IRC with @chx, improved the not-installed check in update_module_enable() to explicitly check for NULL or SCHEMA_UNINSTALLED, which is the same and consistent with drupal_get_installed_schema_version().

Also add a simple test to verify that the default image styles exist if image.module is enabled by user.module.

chx’s picture

chx commented
Status: Needs review » Needs work

This is what #3 asked for, you wrote it just haven't made it available for other modules potentially facing the same problem but also for easier change should another image update appear. Also note I added a return value to update_module_enable:

function image_enable_8002() {
    // Install image.module with schema version 8002 as a previous version
    // would have to create tables that would be removed again.
    $old_schema = update_module_enable(array('image'), 8002);
    if ($old_schema == SCHEMA_UNINSTALLED) {
      config_install_default_config('module', 'image');
      module_load_install('install');
      image_install();
    }
berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Can do a re-roll but wouldn't we solve such a situation with an update dependency? Should this live in image.install? And not sure about the function name, it would be weird to change the version 8002 to something else without changing the function name as well and then we'd have to change it everywhere anyway.

The return value for update_enable_module() looks useful but it would have to be an array as we pass in an array of modules.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Double post.

chx’s picture

chx commented

Oh right, that should be an array. Hrm, dependency, dependency on *what* ? If image is uninstalled you can't depend on an update of it.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Depend on user_update_8011 ?

chx’s picture

chx commented

OH. That. Interesting thought. Could work, yeah. OK, scrap the function but still may I get the return value, please? :)

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs work » Needs review
StatusFileSize
new enable-image-safely-1894328-12.patch3.86 KB

Like this?

chx’s picture

chx commented
Status: Needs review » Needs work

If you'd run $schema_store->get($module) only once it'd be RTBC.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs work » Needs review
StatusFileSize
new enable-image-safely-1894328-14-interdiff.txt1.75 KB
new enable-image-safely-1894328-14.patch4.16 KB

That makes sense, yes :) Also add a @return.

chx’s picture

chx commented
Status: Needs review » Reviewed & tested by the community

I am totally out of complaints, very nice work, thanks.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Assigned: Unassigned » catch

This looks like the kind of issue catch should probably sign off on.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

This currently overlaps with #1848998: Problems with update_module_enable(), which would make the default config installation unnecessary. Not sure which one should get in first.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs review

I think we need to inline the image_install() call rather than loading the file and calling the API function.

Also that currently calls variable_get() indirectly via http://api.drupal.org/api/drupal/core%21includes%21file.inc/function/fil... so not sure what happens when that gets converted to CMI.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Did that. There's still the question about an update-safe version of config_install_default_config(), see the views update issue.

That issue currently does that right in update_module_enable(). Not sure if we should introduce a helper function instead and let callers decide if they want to call it. Maybe it's safe once we fix the critical issue about that function overwriting existing values.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
StatusFileSize
new enable-image-safely-1894328-19.patch4.37 KB
catch’s picture

catch
he/him
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

That looks good. When file variables are updated, that patch will need to add a hook_update_dependences() to user module I guess. Back to RTBC.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

yched’s picture

yched commented 
+++ b/core/modules/user/user.installundefined
@@ -658,7 +658,18 @@ function user_update_8011() {
+    // Install image.module with schema version 8002 as a previous version
+    // would have to create tables that would be removed again.
+    $old_schema = update_module_enable(array('image'), 8002);

The problem with this is that it needs to be manually bumped each time a new upgrade is added in image.module
Took quite a bit of head scratching in #625958: Support Uploading Multiple Files for HTML5 Browsers via #multiple attribute to figure out the problem was in user_update_11() (see #194 / #196 over there), and the head scratching is probably going to repeat for the next update.

Couldn't we just set the schema to "the last update present in image.module", like on a regular module install ?

amateescu’s picture

amateescu commented

I encountered the same problem in #1996714: Convert FileItem and ImageItem to extend EntityReferenceItem and I lost quite some hours before remembering this thread..