Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#20 | enable-image-safely-1894328-19.patch | 4.37 KB | Berdir |
#14 | enable-image-safely-1894328-14.patch | 4.16 KB | Berdir |
#14 | enable-image-safely-1894328-14-interdiff.txt | 1.75 KB | Berdir |
#12 | enable-image-safely-1894328-12.patch | 3.86 KB | Berdir |
#5 | enable-image-safely-1894328-5.patch | 3.52 KB | Berdir |
Comments
Comment #1
catchComment #2
BerdirUsing 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.
Comment #3
chx CreditAttribution: chx commentedNah, just add
image_enable_8002
which jumps image to 8002.Comment #4
BerdirWhat 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.
Comment #5
BerdirPer 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.
Comment #6
chx CreditAttribution: chx commentedThis 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
:Comment #7
BerdirCan 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.
Comment #8
BerdirDouble post.
Comment #9
chx CreditAttribution: chx commentedOh right, that should be an array. Hrm, dependency, dependency on *what* ? If image is uninstalled you can't depend on an update of it.
Comment #10
BerdirDepend on user_update_8011 ?
Comment #11
chx CreditAttribution: chx commentedOH. That. Interesting thought. Could work, yeah. OK, scrap the function but still may I get the return value, please? :)
Comment #12
BerdirLike this?
Comment #13
chx CreditAttribution: chx commentedIf you'd run $schema_store->get($module) only once it'd be RTBC.
Comment #14
BerdirThat makes sense, yes :) Also add a @return.
Comment #15
chx CreditAttribution: chx commentedI am totally out of complaints, very nice work, thanks.
Comment #16
webchickThis looks like the kind of issue catch should probably sign off on.
Comment #17
BerdirThis 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.
Comment #18
catchI 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.
Comment #19
BerdirDid 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.
Comment #20
BerdirComment #21
catchThat 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.
Comment #22
webchickCommitted and pushed to 8.x. Thanks!
Comment #24
yched CreditAttribution: yched commentedThe 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 ?
Comment #25
amateescu CreditAttribution: amateescu commentedI encountered the same problem in #1996714: Convert FileItem and ImageItem to extend EntityReferenceItem and I lost quite some hours before remembering this thread..