Updated: Comment #33

Problem/Motivation

Image styles are not upgraded from Drupal 7 after the image style backport in Drupal 7.23

Proposed resolution

Fix the image module upgrade path, and improve the upgrade path tests to catch the errors.

Remaining tasks

  • Create patch to use container instead of \Drupal::config per meta issue in the related issues section.
  • Patch review

User interface changes

None

API changes

None

Original report by niko-

Hi

My environment is migrated d7 to d8 site

I found empty image styles labels in admin /admin/config/media/image-styles

See attached screanshort

Files: 
CommentFileSizeAuthor
#48 drupal8.image-system.2049465-48-FAIL.patch306.82 KBclaudiu.cristea
FAILED: [[SimpleTest]]: [MySQL] 58,554 pass(es), 5 fail(s), and 4 exception(s).
[ View ]
#48 drupal8.image-system.2049465-48.patch313.08 KBclaudiu.cristea
PASSED: [[SimpleTest]]: [MySQL] 58,625 pass(es).
[ View ]
#48 interdiff-against-46.patch5.52 KBclaudiu.cristea
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch interdiff-against-46.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#48 styles-d8-good.png42.65 KBclaudiu.cristea
#46 drupal8.image-system.2049465-45.patch307.91 KBclaudiu.cristea
PASSED: [[SimpleTest]]: [MySQL] 58,714 pass(es).
[ View ]
#43 styles-d7.png74.55 KBclaudiu.cristea
#43 styles-d8.png28.18 KBclaudiu.cristea
#43 drupal8.image-system.2049465-43.patch309.22 KBclaudiu.cristea
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8.image-system.2049465-43.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#43 interdiff.txt514 bytesclaudiu.cristea
#41 drupal-2049465-image-upgrade-path-41.patch307.21 KBLinL
PASSED: [[SimpleTest]]: [MySQL] 59,652 pass(es).
[ View ]
#35 interdiff.txt1.15 KBmradcliffe
#35 drupal-2049465-image-upgrade-path-35.patch307.26 KBmradcliffe
PASSED: [[SimpleTest]]: [MySQL] 58,550 pass(es).
[ View ]
#32 interdiff.txt15.87 KBmradcliffe
#31 drupal-2049465-image-upgrade-path-31.patch307.18 KBmradcliffe
PASSED: [[SimpleTest]]: [MySQL] 57,911 pass(es).
[ View ]
#31 interdiff.txt1.29 KBmradcliffe
#29 2049465-image_style-upgrade-29.patch307.18 KBmradcliffe
FAILED: [[SimpleTest]]: [MySQL] 56,859 pass(es), 18 fail(s), and 0 exception(s).
[ View ]
#27 2049465-image_style-upgrade-27-FAIL.patch305.69 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 58,162 pass(es), 4 fail(s), and 4 exception(s).
[ View ]
#27 2049465-image_style-upgrade-27.patch307.19 KBandypost
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 2049465-image_style-upgrade-27.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#23 2049465-image_style-upgrade-23.patch307.19 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 58,029 pass(es).
[ View ]
#23 2049465-image_style-upgrade-23-do-not-test.txt5.08 KBandypost
#23 interdiff-db.txt998 bytesandypost
#23 interdiff.txt2.03 KBandypost
#20 2049465-image_style-upgrade-20.patch4.61 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 57,777 pass(es).
[ View ]
#17 2049465-image_style-upgrade-17.patch1.69 KBniko-
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#17 2049465-image_style-upgrade-test-17.patch2.9 KBniko-
FAILED: [[SimpleTest]]: [MySQL] 57,520 pass(es), 4 fail(s), and 4 exception(s).
[ View ]
#15 2049465-image_style-upgrade-15.patch4.56 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 57,517 pass(es).
[ View ]
#14 image.2049465.Upgrade_of_image_styles_and_effects_broken_fix.14.d8.patch977 bytesniko-
FAILED: [[SimpleTest]]: [MySQL] 57,478 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#10 image.2049465.Upgrade_of_image_styles_and_effects_broken_fix.10.d8.patch955 bytesniko-
FAILED: [[SimpleTest]]: [MySQL] 57,161 pass(es), 1 fail(s), and 2 exception(s).
[ View ]
#1 image.2049465.empty_image_styles_labels_fix.1.d8.patch820 bytesniko-
FAILED: [[SimpleTest]]: [MySQL] 57,329 pass(es), 9 fail(s), and 7 exception(s).
[ View ]
экрана от 2013-07-24 18:18:16.png73.54 KBniko-

Comments

StatusFileSize
new820 bytes
FAILED: [[SimpleTest]]: [MySQL] 57,329 pass(es), 9 fail(s), and 7 exception(s).
[ View ]

Patch attached

Priority:Normal» Critical
Issue tags:+Needs upgrade path tests

If this is in fact the case, it is certainly critical. It also means we're missing upgrade path tests in for this.

Forgot the most important part: Thanks @niko- for testing the upgrade path so thoroughly and reporting this issue so early in the cycle. Awesomesauce!!!

Thanks @tstoeckler I try to migrate real miltilengual site to d8 so this is not the last issue.
One more issue about image styles admin https://drupal.org/node/2049915

This should probably be combined with image_update_8000(). We can still do that until beta1.

thanks tim

D7 image style labels landed #606598: Human readable image-style names so probably upgrade path should be changed accordingly

Title:Empty image styles labels in admin after d7 to d8 upgradeUpgrade of image styles and effects broken

StatusFileSize
new955 bytes
FAILED: [[SimpleTest]]: [MySQL] 57,161 pass(es), 1 fail(s), and 2 exception(s).
[ View ]

image_ipdate_8000 patch attached

core/modules/system/lib/Drupal/system/Tests/Upgrade/ImageUpgradePathTest.php should be fixed accordingly

Status:Active» Needs review

let's see whats broken

Status:Needs review» Needs work

Status:Needs work» Active
StatusFileSize
new977 bytes
FAILED: [[SimpleTest]]: [MySQL] 57,478 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

patch fix

Status:Active» Needs review
StatusFileSize
new4.56 KB
PASSED: [[SimpleTest]]: [MySQL] 57,517 pass(es).
[ View ]

Proper fix for upgrade path and test

+++ b/core/modules/system/lib/Drupal/system/Tests/Upgrade/ImageUpgradePathTest.phpundefined
@@ -68,10 +74,10 @@ public function testImageStyleUpgrade() {
+      $configured_style = $this->container->get('config.factory')->get('image.style.' . $name)->get();

Let's move the config.factory into a variable outside the foreach. Otherwise, looks good.

When you post the next patch, can you also post a test-only patch to see it fail? Thanks!

StatusFileSize
new2.9 KB
FAILED: [[SimpleTest]]: [MySQL] 57,520 pass(es), 4 fail(s), and 4 exception(s).
[ View ]
new1.69 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

splited patches with move the config.factory outside the foreach.

@niko-, that's backwards, that's just the fix. We need just the test.

Status:Needs review» Needs work

The last submitted patch, 2049465-image_style-upgrade-test-17.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new4.61 KB
PASSED: [[SimpleTest]]: [MySQL] 57,777 pass(es).
[ View ]

#17 shows the test failure
so just combined patch - s/factory/config_factory

PS: related #1966436: Default *content* entity languages are not set for entities created with the API

@@ -147,22 +146,23 @@ function _image_update_get_style_with_effects(array $style) {
+      // @todo Replace with $style['label'] after Drupal 7.23 released.

This happened

Closed #2062341: Fix image upgrade path to work with image-style name backport as a dupe

Thanks, tim.

I think the patch in #20 makes sense. However I am wondering if it wouldn't be better to use image_style_load() instead of config_factory to load the configuration entity.

StatusFileSize
new2.03 KB
new998 bytes
new5.08 KB
new307.19 KB
PASSED: [[SimpleTest]]: [MySQL] 58,029 pass(es).
[ View ]

And here's a combined patch with #2062341-4: Fix image upgrade path to work with image-style name backport and changed upgrade database damps in drupal-7.filled.standard_all.database.php.gz and drupal-7.bare.standard_all.database.php.gz (added label field)

So closed #606598-216: Human readable image-style names

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

The last submitted patch, 2049465-image_style-upgrade-23.patch, failed testing.

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

#23: 2049465-image_style-upgrade-23.patch queued for re-testing.

  1. +++ b/core/modules/image/image.install
    @@ -146,22 +146,23 @@ function _image_update_get_style_with_effects(array $style) {
    -    $config = config('image.style.' . $name);
    -    $config->set('name', $name);
    -    $config->set('effects', $style['effects']);
    ...
    +      ->set('name', $style['name'])
    ...
    +      ->set('effects', _image_update_get_style_with_effects($style))

    This makes the patch harder to read. Can you please leave the coding style alone and just change what is needed?

  2. +++ b/core/modules/system/lib/Drupal/system/Tests/Upgrade/ImageUpgradePathTest.php
    @@ -67,11 +73,12 @@ public function testImageStyleUpgrade() {
    -      $config = config('image.style.' . $name);
    +      $configured_style = $config_factory->get('image.style.' . $name)->get();
    ...
    -      foreach ($config->get('effects') as $uuid => $effect) {
    +      foreach ($configured_style['effects'] as $uuid => $effect) {

    Same here

Also, please post one with just tests to prove the upgrade path tests work.

StatusFileSize
new307.19 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 2049465-image_style-upgrade-27.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new305.69 KB
FAILED: [[SimpleTest]]: [MySQL] 58,162 pass(es), 4 fail(s), and 4 exception(s).
[ View ]

The changes not about code style but about re-write implementation of update:
1) no need to fetch all styles to memory and no need in 2 loops
2) optimize a test to not make get() of config

#17 2049465-image_style-upgrade-test-17.patch is just a test, that points broken upgrade that's was pointed in #20 but see attached file

Also re-roll for current HEAD

@@ -67,11 +73,12 @@ public function testImageStyleUpgrade() {
-      $config = config('image.style.' . $name);
+      $configured_style = $config_factory->get('image.style.' . $name)->get();
...
-      foreach ($config->get('effects') as $uuid => $effect) {
+      foreach ($configured_style['effects'] as $uuid => $effect) {
@@ -79,9 +86,13 @@ public function testImageStyleUpgrade() {
-      $this->assertEqual($this->sortByKey($style), $config->get(), format_string('@first is equal to @second.', array(
...
+      $this->assertTrue($configured_style['uuid'], 'UUID assigned to converted style.');
...
+      $style['uuid'] = $configured_style['uuid'];
+      $this->assertEqual($this->sortByKey($style), $this->sortByKey($configured_style), format_string('@first is equal to @second.', array(
...
+        '@second' => var_export($this->sortByKey($configured_style), TRUE),

I see no reason get() each property

Status:Needs review» Needs work

The last submitted patch, 2049465-image_style-upgrade-27.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new307.18 KB
FAILED: [[SimpleTest]]: [MySQL] 56,859 pass(es), 18 fail(s), and 0 exception(s).
[ View ]

Re-roll patch. I ran UpdateLocaleTest, but it did not return any failures locally.

Note that the following is different from patch #27:

@@ -68,10 +74,10 @@ public function testImageStyleUpgrade() {
       ),
     );
     foreach ($expected_styles as $name => $style) {
-      $config = \Drupal::config('image.style.' . $name);
+      $configured_style = \Drupal::config('image.style.' . $name)->get();
       // Replace placeholder with image effect name keys with UUID's generated
       // during by the image style upgrade functions.
-      foreach ($config->get('effects') as $uuid => $effect) {
+      foreach ($configured_style['effects'] as $uuid => $effect) {
         // Copy placeholder data.
         $style['effects'][$uuid] = $style['effects'][$effect['id']];
         // Set the missing uuid key as this is unknown because it is a UUID.

Status:Needs review» Needs work

The last submitted patch, 2049465-image_style-upgrade-29.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.29 KB
new307.18 KB
PASSED: [[SimpleTest]]: [MySQL] 57,911 pass(es).
[ View ]

Well, I mucked that patch up pretty bad.

Re-based, re-applied #29, and replaced the database dumps from patch #27.

Edit: Dangit, wrong interdiff.txt

StatusFileSize
new15.87 KB

Attached correct interdiff.txt.

Just a small nitpick - test should use own container

@@ -68,10 +74,10 @@ public function testImageStyleUpgrade() {
+      $configured_style = \Drupal::config('image.style.' . $name)->get();

please revert this back to $this->container

Issue summary:View changes

Add issue summary update.

StatusFileSize
new307.26 KB
PASSED: [[SimpleTest]]: [MySQL] 58,550 pass(es).
[ View ]
new1.15 KB

Revert change in #31 to #27 per meta issue.

#27: 2049465-image_style-upgrade-27.patch queued for re-testing.

Status:Needs review» Reviewed & tested by the community

#17 and #27 shows that test is valid, also tested manually on live D7 site

Status:Reviewed & tested by the community» Needs review

You can't RTBC this, it's half your code.

Issue tags:+beta blocker

Tagging all critical D8 upgrade path issues as "beta blocker."

StatusFileSize
new307.21 KB
PASSED: [[SimpleTest]]: [MySQL] 59,652 pass(es).
[ View ]

Patch no longer applied, here's a reroll.

Status:Needs review» Needs work

I tried to manually test this issue. When running update.php the process breaks with:

An AJAX HTTP error occurred.
HTTP Result Code: 200
Debugging information follows.
Path: http://localhost:8888/7test/core/update.php?op=selection&token=7TNfIokQJyZ06g7rK5ch6CCWL_-kzGbAbmpYlfPYdac&id=3&op=do_nojs&op=do
StatusText: OK
ResponseText:
Fatal error:  Class 'Uuid' not found in /Users/clau/Documents/development/8/8test/core/modules/image/image.install on line 101

Indeed, I see no use Drupal\Component\Uuid\Uuid in image.install header. But, anyway, it seems we have a brand new Uuid service since #1969572: Make Uuid a service.

Assigned:Unassigned» claudiu.cristea
Status:Needs work» Needs review
StatusFileSize
new514 bytes
new309.22 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8.image-system.2049465-43.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new28.18 KB
new74.55 KB

This one passes manual test.

But I see another problem. Styles shipped with Drupal are missed in D8.

D7 styles

styles-d7.png

D8 styles

styles-d8.png

And we need them!

Setting to "needs review" only to see tests. Taking the issue just to keep track.

Status:Needs review» Needs work

The last submitted patch, drupal8.image-system.2049465-43.patch, failed testing.

Status:Needs work» Needs review

I posted a wrong patch in #43. Interdiff and all other remain.

StatusFileSize
new307.91 KB
PASSED: [[SimpleTest]]: [MySQL] 58,714 pass(es).
[ View ]

Ups! The patch ;)

And here is the explanation: Styles shipped with D7 (defined in image_image_default_styles()) are not filled in database (tables image_styles and image_effects) until they are not saved for the first time. The upgrade path is scanning only the image_styles table, without taking into considerations those from code that are not filled also in DB. The update script needs more attention and work. Will try something tomorrow.

StatusFileSize
new42.65 KB
new5.52 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch interdiff-against-46.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new313.08 KB
PASSED: [[SimpleTest]]: [MySQL] 58,625 pass(es).
[ View ]
new306.82 KB
FAILED: [[SimpleTest]]: [MySQL] 58,554 pass(es), 5 fail(s), and 4 exception(s).
[ View ]
  • Now the patch is importing also un-overwritten image styles.
  • Attached the "only-test" patch to prove the bug.
  • Interdiff against #46.
  • Tested also manually. You can see screenshots from D7 vs. D8 taken during the manual tests.

D7 styles

styles-d7.png

D8 imported styles

styles-d8-good.png

Status:Needs review» Needs work

The last submitted patch, interdiff-against-46.patch, failed testing.

Status:Needs work» Needs review

Poorly named the interdiff. Setting back to "needs review".

Status:Needs review» Needs work

The last submitted patch, interdiff-against-46.patch, failed testing.

Status:Needs work» Needs review

This is ready for RTBC or more reviews.

Status:Needs review» Reviewed & tested by the community

I think this rtbc! Anyway we can't access d7 styles that shiped with features or custom module because hooks are not allowed in update.

Status:Reviewed & tested by the community» Fixed

Awesome work on this. Thanks so much!!

Committed and pushed to 8.x.

Assigned:claudiu.cristea» Unassigned
Issue tags:-beta blocker

Great! Untagging and removed myself.

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

Issue summary:View changes

Update from andypost's comments.