Posted by heyrocker on March 2, 2012 at 7:10pm
21 followers
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | configuration system |
| Category: | task |
| Priority: | critical |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
| Issue tags: | Configuration system, D8 upgrade path |
Issue Summary
Problem
- We have implemented a shiny new configuration system, and its first core implementations are the system performance settings and image styles. Unfortunately we did not create a D7 upgrade path for image styles to the new system.
Goal
- Sites upgrading from D7 will have their image styles automatically moved to the new configuration system.
Details
- Install default config
- If default image styles have been overridden, then mirror those changes in the new system.
- Migrate any image styles defined in the database through the UI
Proposed resolution
- Implement the above.
Notes
- I don't think we should take on the task of migrating image styles defined in hook_default_image_styles(). It's going to be next to impossible to guarantee since we would need those modules to be enabled, and I think it is up to those module owners to manage their own upgrades.
Comments
#1
This sounds like an opportunity to write the tests first. I feel like I'm going to take a sickday tomorrow. I'll look at writing those then.
#2
#3
tagging
#4
#5
This patch creates a d7 test db that has overridden the 'thumbnail' image style, and also adds a new custom image style. No upgrade path yet, but this should make it easier to write the eventual tests needed. Note to anybody writing upgrade path tests, the instructions here, while cumbersome, are quite thorough.
#6
Err, the actual patch.
#7
Sending to the bot.
#8
Setting back to needs work since the patch in #6 is just a start. Going to try to push this further today.
#9
I have an in-progress patch on this I can post in about an hour or so here, ust need to finish a call.
#10
So... five days later... I have pushed all my work on this to the branch 1464554_upgrade_path in the CMI sandbox at
http://drupalcode.org/sandbox/heyrocker/1145636.git
Some notes:
- This also includes some work from #1464944: Installation of configuration system fails in some cases that can be ignored. Most of what relates to this issue is in system.install.
- The function _system_get_variables_and_defaults() can go away, you should be able to use update_variables_to_config() instead (see #1497132: Provide a generalised function to update variables from Drupal 7 to Drupal 8's configuration management system)
Thanks! Sorry about the delay getting this up.
#11
Did a little bit more work on this. This patch adds the same test db as in #6, and also a test db for system settings, plus upgrade tests verifying those. Still needs work on the image upgrade path and tests for that.
#12
This patch has update functionality for image styles. The tests themselves hard-code the expected array, which I'm not sure is the best way to go, but I wasn't sure how else to test the successful upgrade.
#13
The last submitted patch, 1464554-12-system-image-upgrade-path.patch, failed testing.
#14
The failures are related to #1500312: Create $config_directory_name during a D7 => D8 upgrade.
Also this still seems to include the changes from #1464944: Installation of configuration system fails in some cases.
Sorry this isn't a proper review really, but I will get back to it in the next couple days. We're held up on the test failures for now anyways.
#15
There are several changes going on that will impact this patch, most notably #1464944: Installation of configuration system fails in some cases which changes the way that the installation stuff works and #1470824: XML encoder can only handle a small subset of PHP arrays, so switch to YAML which is probably going to change the files around and remove the entire need for there to be machine names in the image effects. I think we should hold off on this patch until those sort themselves out. I'm trying to focus on getting those rolled out because they have lots of implications, so hopefully we can come back to this soon. Marking postponed until then though.
#16
Upgrade path for Image styles should copy style's name to label if #606598-96: Human readable image-style names gets commited
#17
Should no longer be postponed since the essential parts of #1470824: XML encoder can only handle a small subset of PHP arrays, so switch to YAML is in.
#18
#19
I'm not able to distill the actual changes for this issue from the patch in #12.
It pretty much looks like we need to start from scratch with the patch in #6, which contains a D7 database dump for upgrade path testing only.
#20
OK. going to work on that tonight.
#21
Whoaha! Took more time than expected. Lots of patch comparing. Only this part:
$module_styles = module_invoke('image', 'image_default_styles');foreach ($module_styles as $style_name => $style) {
$style['name'] = $style_name;
foreach ($style['effects'] as $key => $effect) {
$definition = image_effect_definition_load($effect['name']); // Need to remake this
$effect = array_merge($definition, $effect);
$style['effects'][$key] = $effect;
}
$styles[$style_name] = $style;
}
Needs some work, it throws a notice :) But all the tests seem to work. Nice!
#22
Looking at that it's obvious this isn't going to work as the hook is removed. Btw we forgot to remove the hook from the api docs very confusing...
#23
The last submitted patch, 1464554-21-system-image-upgrade-path.patch, failed testing.
#24
This works locally :)
#25
#26
It's green!
Needs manual testing:
1) Install a drupal 7 site and change at least one image style and change at least one system performance option
2) Apply this patch and #1500312: Create $config_directory_name during a D7 => D8 upgrade to drupal8
3) Upgrade and see if your custom settings are preserved
#27
With settings.php set to 644, update.php gripes about settings.php being read-only, as expected.
With settings.php set to 666, update.php gives WSOD. No related errors reported or logged.
Environment: PHP 5.3.8, apache 2.2.21, OSX 10.6.8
It did throw an error about missing favicon.ico, `touch favicon.ico` removed this error but update.php is still throwing WSOD.
[Fri Jul 06 12:30:50 2012] [error] [client 127.0.0.1] File does not exist: /Users/ogredude/Sites/d8sandbox/favicon.ico
[Fri Jul 06 12:30:50 2012] [error] [client 127.0.0.1] Uncaught PHP Exception Symfony\\Component\\HttpKernel\\Exception\\NotFoundHttpException: "No route found for "GET /favicon.ico"" at /Users/ogredude/Sites/d8sandbox/core/lib/Drupal/Core/EventSubscriber/RouterListener.php line 88
[EDIT]
OK, so it turns out that update.php goes WSOD if files/ is not writable. I'll be opening an issue for that one.
Once I corrected that issue, the rest of the update went smoothly, and the new image styles and the performance settings survived intact.
#28
After incrementing the update hook number, this patch worked as expected for me. Here's what I did:
I changed one number, so no interdiff this time.
#29
The last submitted patch, drupal-system_image_upgrade_path-1464554-28.patch, failed testing.
#30
Attached is patch 24 with update hook incremented. Ignore the above one, I merged the two patches accidentally.
#31
The last submitted patch, drupal-system_image_upgrade_path-1464554-29.patch, failed testing.
#32
WTF
Fatal error: Call to undefined function Drupal\system\Tests\Upgrade\language_negotiation_include() in /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/system/lib/Drupal/system/Tests/Upgrade/LanguageUpgradePathTest.php on line 144FATAL Drupal\system\Tests\Upgrade\LanguageUpgradePathTest: test runner returned a non-zero error code (255).
- Found database prefix 'simpletest919155' for test ID 458.
[06-Jul-2012 20:07:23] PHP Fatal error: Call to undefined function Drupal\system\Tests\Upgrade\language_negotiation_include() in /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/system/lib/Drupal/system/Tests/Upgrade/LanguageUpgradePathTest.php on line 144
Why? is this failing here, isn't related at all. And why is the procedural function looking inside the Drupal\system\Tests\Upgrade\ direcotry...
#33
#30: drupal-system_image_upgrade_path-1464554-29.patch queued for re-testing.
#34
The last submitted patch, drupal-system_image_upgrade_path-1464554-29.patch, failed testing.
#35
Well, the issue title is about system and image, and it adds drupal-7.image.database.php and drupal-7.system.database.php accordingly.
But, it adds LanguageUpgradePathTest.php and makes no changes to the test DB files, and they don't know about the addition of language yet.
So
a) Why is language part of this issue?
b) If it should be, can someone update drupal-7.language.database.php?
#36
Er, hm. It does not add it. My repo must have been out of date? Still very bizarre. Same problem as #1496462-60: Convert RSS publishing settings to configuration system
#37
Hmm, very veyr strange, lets see what happens with these patches...
#38
damnit...
#39
The last submitted patch, drupal-system_image_upgrade_path-1464554-37_b.patch, failed testing.
#40
#41
The last submitted patch, drupal8.config-upgrade.40.patch, failed testing.
#42
Whops wrong patch
#43
Finally green again, stupid test bots...
Based on #27
And #28
This is RTBC. But this can use a code review.
#44
788e22f Various fixes and clean-ups.
#45
#46
The last submitted patch, config.upgrade.44.patch, failed testing.
#47
b59435f Dear testing framework, please DIE.
#48
The last submitted patch, config.upgrade.47.patch, failed testing.
#49
array ('name' => 'thumbnail',
'effects' =>
array (
'image_scale_177_177_0' =>
array (
'name' => 'image_scale',
'data' =>
array (
'width' => '177',
'height' => '177',
'upscale' => '0',
),
'ieid' => 'image_scale_177_177_0',
'weight' => '0',
),
),
) is equal to array (
'effects' =>
array (
'image_desaturate' =>
array (
'data' =>
array (
),
'ieid' => 'image_desaturate',
'name' => 'image_desaturate',
'weight' => '2',
),
'image_scale_177_177_0' =>
array (
'data' =>
array (
'height' => '177',
'upscale' => '0',
'width' => '177',
),
'ieid' => 'image_scale_177_177_0',
'name' => 'image_scale',
'weight' => '0',
),
),
'name' => 'thumbnail',
)
The image_desaturate effect is missing for some reason.
#50
Note that the format of image styles is going to be changing soon, with a new method of determining effect IDs at the very least (#1508872: Image effects duplicate on edit), and most likely some revamping of the image style/effect APIs down the road. This should probably be postponed until those issues settle out because large portions of this will have to be redone once that happens.
#51
#1508872: Image effects duplicate on edit landed, let's get back to this! :)
#52
Patch attached fixes test failure in #47 - test was adding desaturate effect to default thumbnail image style.
It removes system config variable upgrade tests as these are now handled in #1789606: Upgrade path tests for all system module variables converted to CMI so far... will issue summary to reflect.
#53
Fixing title...
#54
+++ b/core/modules/image/image.installundefined@@ -103,3 +103,68 @@ function image_requirements($phase) {
+ foreach ($effect['data'] as $key => $value) {
+ $effect['ieid'] .= '_' . $value;
+ }
+ $effect['ieid'] = preg_replace('@[^a-zA-Z0-9_-]@', '', $effect['ieid']);
+
+ $effects[$effect['ieid']] = $effect;
we're using uuid for this:
<?phpif (empty($effect['ieid'])) {
$uuid = new Uuid();
$effect['ieid'] = $uuid->generate();
}
?>
If there a reason not use use
image_effect_save?#55
Good catch on the new effect UUID naming! New patch to address issues raised in #54 - we now create a new UUID for each upgraded effect.
The reason we can't use
image_effect_save- quoting from @sun's todo in the patch#56
+++ b/core/modules/image/image.installundefined@@ -103,3 +105,68 @@ function image_requirements($phase) {
+ // @todo Module updates are not able to invoke module hooks and API functions;
+ // additional information previously supplied via hook_image_effect_info()
+ // is not and cannot be taken into account.
Is this actually a problem? The data for an instance of the effect in a style will be serialized into {image_effects} and converted here. It will be stranded if the module defining the effect doesn't get upgraded but that isn't our concern I don't think.
+++ b/core/modules/system/tests/upgrade/drupal-7.image.database.phpundefined@@ -0,0 +1,59 @@
+ * @file
+ * Database additions for language tests. Used in upgrade.language.test.
+ *
+ * This dump only contains data and schema components relevant for language
+ * functionality. The drupal-7.filled.database.php file is imported before
+ * this dump, so the two form the database structure expected in tests
+ * altogether.
This comment needs to be fixed.
Running tests locally I got a fail, gonna rerun it on the bot to see if it gets reproduced.
#57
#55: 1464554_55..drupal8.image-style-upgrade.patch queued for re-testing.
#58
The last submitted patch, 1464554_55..drupal8.image-style-upgrade.patch, failed testing.
#59
Yeah something is going on with the test-custom style, the thumbnail tests are working. Unfortunately I don't have time to dig into that atm.
#60
The attached patch implements suggestions from #56 and uses a more reliable way to map uuids to effects for the upgrade test.
#61
Ship that thing!
#62
Looks good to me, and comes with tests. Committed to 8.x.
#63
Automatically closed -- issue fixed for 2 weeks with no activity.