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.

Files: 
CommentFileSizeAuthor
#20 enable-image-safely-1894328-19.patch4.37 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 49,187 pass(es).
[ View ]
#14 enable-image-safely-1894328-14.patch4.16 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 50,113 pass(es).
[ View ]
#14 enable-image-safely-1894328-14-interdiff.txt1.75 KBBerdir
#12 enable-image-safely-1894328-12.patch3.86 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 49,591 pass(es).
[ View ]
#5 enable-image-safely-1894328-5.patch3.52 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 49,391 pass(es).
[ View ]
#5 enable-image-safely-1894328-5-interdiff.txt1.62 KBBerdir
#4 enable-image-safely-1894328-4.patch2.77 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 49,279 pass(es).
[ View ]

Comments

Priority:Major» Critical
Issue tags:+D8 upgrade path

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.

Nah, just add image_enable_8002 which jumps image to 8002.

Status:Active» Needs review
StatusFileSize
new2.77 KB
PASSED: [[SimpleTest]]: [MySQL] 49,279 pass(es).
[ View ]

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.

StatusFileSize
new1.62 KB
new3.52 KB
PASSED: [[SimpleTest]]: [MySQL] 49,391 pass(es).
[ View ]

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.

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:

<?php
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();
    }
?>

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.

Double post.

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.

Depend on user_update_8011 ?

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

Status:Needs work» Needs review
StatusFileSize
new3.86 KB
PASSED: [[SimpleTest]]: [MySQL] 49,591 pass(es).
[ View ]

Like this?

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new1.75 KB
new4.16 KB
PASSED: [[SimpleTest]]: [MySQL] 50,113 pass(es).
[ View ]

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

Status:Needs review» Reviewed & tested by the community

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

Assigned:Unassigned» catch

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

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.

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.

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.

StatusFileSize
new4.37 KB
PASSED: [[SimpleTest]]: [MySQL] 49,187 pass(es).
[ View ]

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.

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.

+++ 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 ?

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