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.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

cosmicdreams’s picture

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.

catch’s picture

Issue tags: +D8 upgrade path
gdd’s picture

Issue tags: +Configuration system

tagging

chx’s picture

Category: bug » task
jhedstrom’s picture

Status: Active » Needs work
FileSize
0 bytes

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.

jhedstrom’s picture

FileSize
3.38 KB

Err, the actual patch.

xjm’s picture

Status: Needs work » Needs review
Issue tags: +Needs tests

Sending to the bot.

jhedstrom’s picture

Assigned: Unassigned » jhedstrom
Status: Needs review » Needs work

Setting back to needs work since the patch in #6 is just a start. Going to try to push this further today.

gdd’s picture

I have an in-progress patch on this I can post in about an hour or so here, ust need to finish a call.

gdd’s picture

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.

jhedstrom’s picture

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.

jhedstrom’s picture

Status: Needs work » Needs review
FileSize
5.09 KB
37.79 KB

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.

Status: Needs review » Needs work

The last submitted patch, 1464554-12-system-image-upgrade-path.patch, failed testing.

gdd’s picture

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.

gdd’s picture

Status: Needs work » Postponed

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.

andypost’s picture

Upgrade path for Image styles should copy style's name to label if #606598-96: Human readable image-style names gets commited

cosmicdreams’s picture

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.

cosmicdreams’s picture

Status: Postponed » Needs work
sun’s picture

Title: Implement an upgrade path for performance settings and image styles to the config system » Upgrade path for system.performance and image style configuration

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.

cosmicdreams’s picture

Assigned: jhedstrom » cosmicdreams

OK. going to work on that tonight.

aspilicious’s picture

Status: Needs work » Needs review
FileSize
13.29 KB

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!

aspilicious’s picture

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...

Status: Needs review » Needs work

The last submitted patch, 1464554-21-system-image-upgrade-path.patch, failed testing.

aspilicious’s picture

This works locally :)

aspilicious’s picture

Status: Needs work » Needs review
aspilicious’s picture

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

Ogredude’s picture

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.

disasm’s picture

After incrementing the update hook number, this patch worked as expected for me. Here's what I did:

  • Installed Drupal 7
  • Modified large image style to 500x400
  • Set cached pages for anonmymous users, expiration of cached pages to 1 min, and compress cached pages
  • Installed Drupal 8
  • git apply --index -v 1464554-24-system-image-upgrade-path.patch
  • git apply --index -v 1500312-fixed-config-upgrade-29.patch
  • increment update hook (see attached patch)
  • ran update.php
  • checked image style and verified large image was 500x400
  • checked performance and verified settings were correct

I changed one number, so no interdiff this time.

Status: Needs review » Needs work

The last submitted patch, drupal-system_image_upgrade_path-1464554-28.patch, failed testing.

disasm’s picture

Status: Needs work » Needs review
FileSize
11.81 KB

Attached is patch 24 with update hook incremented. Ignore the above one, I merged the two patches accidentally.

Status: Needs review » Needs work

The last submitted patch, drupal-system_image_upgrade_path-1464554-29.patch, failed testing.

aspilicious’s picture

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 144
FATAL 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...

tim.plunkett’s picture

Status: Needs work » Needs review
Issue tags: -Needs manual testing, -D8 upgrade path, -Configuration system

Status: Needs review » Needs work
Issue tags: +Needs manual testing, +D8 upgrade path, +Configuration system

The last submitted patch, drupal-system_image_upgrade_path-1464554-29.patch, failed testing.

tim.plunkett’s picture

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?

tim.plunkett’s picture

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

aspilicious’s picture

Hmm, very veyr strange, lets see what happens with these patches...

aspilicious’s picture

Status: Needs work » Needs review

damnit...

Status: Needs review » Needs work

The last submitted patch, drupal-system_image_upgrade_path-1464554-37_b.patch, failed testing.

aspilicious’s picture

Status: Needs work » Needs review
FileSize
8.88 KB

Status: Needs review » Needs work

The last submitted patch, drupal8.config-upgrade.40.patch, failed testing.

aspilicious’s picture

Status: Needs work » Needs review
FileSize
13.51 KB

Whops wrong patch

aspilicious’s picture

Issue tags: -Needs manual testing

Finally green again, stupid test bots...

Based on #27

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.

And #28

  • Installed Drupal 7
  • Modified large image style to 500x400
  • Set cached pages for anonmymous users, expiration of cached pages to 1 min, and compress cached pages
  • Installed Drupal 8
  • git apply --index -v 1464554-24-system-image-upgrade-path.patch
  • git apply --index -v 1500312-fixed-config-upgrade-29.patch
  • increment update hook (see attached patch)
  • ran update.php
  • checked image style and verified large image was 500x400
  • checked performance and verified settings were correct

This is RTBC. But this can use a code review.

sun’s picture

FileSize
10.41 KB

788e22f Various fixes and clean-ups.

sun’s picture

FileSize
12.76 KB

Status: Needs review » Needs work

The last submitted patch, config.upgrade.44.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
897 bytes
10.49 KB

b59435f Dear testing framework, please DIE.

Status: Needs review » Needs work

The last submitted patch, config.upgrade.47.patch, failed testing.

sun’s picture

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.

gdd’s picture

Status: Needs work » Postponed

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.

sun’s picture

Status: Postponed » Needs work

#1508872: Image effects duplicate on edit landed, let's get back to this! :)

alexpott’s picture

Assigned: cosmicdreams » Unassigned
Status: Needs work » Needs review
FileSize
6.49 KB

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.

alexpott’s picture

Title: Upgrade path for system.performance and image style configuration » Upgrade path for image style configuration

Fixing title...

attiks’s picture

Status: Needs review » Needs work
+++ 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:

  if (empty($effect['ieid'])) {
    $uuid = new Uuid();
    $effect['ieid'] = $uuid->generate();
   }

If there a reason not use use image_effect_save?

alexpott’s picture

Status: Needs work » Needs review
FileSize
7.63 KB
4.24 KB

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

Module updates are not able to invoke module hooks and API functions

gdd’s picture

Status: Needs review » Needs work
+++ 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.

gdd’s picture

Status: Needs work » Needs review
Issue tags: -D8 upgrade path, -Configuration system

Status: Needs review » Needs work
Issue tags: +D8 upgrade path, +Configuration system

The last submitted patch, 1464554_55..drupal8.image-style-upgrade.patch, failed testing.

gdd’s picture

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.

alexpott’s picture

Status: Needs work » Needs review
FileSize
7.39 KB
3.62 KB

The attached patch implements suggestions from #56 and uses a more reliable way to map uuids to effects for the upgrade test.

gdd’s picture

Status: Needs review » Reviewed & tested by the community

Ship that thing!

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Looks good to me, and comes with tests. Committed to 8.x.

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary to remove system site.performance stuff.