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

CommentFileSizeAuthor
#48 drupal8.image-system.2049465-48-FAIL.patch306.82 KBclaudiu.cristea
#48 drupal8.image-system.2049465-48.patch313.08 KBclaudiu.cristea
#48 interdiff-against-46.patch5.52 KBclaudiu.cristea
#48 styles-d8-good.png42.65 KBclaudiu.cristea
#46 drupal8.image-system.2049465-45.patch307.91 KBclaudiu.cristea
#43 styles-d7.png74.55 KBclaudiu.cristea
#43 styles-d8.png28.18 KBclaudiu.cristea
#43 drupal8.image-system.2049465-43.patch309.22 KBclaudiu.cristea
#43 interdiff.txt514 bytesclaudiu.cristea
#41 drupal-2049465-image-upgrade-path-41.patch307.21 KBLinL
#35 interdiff.txt1.15 KBmradcliffe
#35 drupal-2049465-image-upgrade-path-35.patch307.26 KBmradcliffe
#32 interdiff.txt15.87 KBmradcliffe
#31 drupal-2049465-image-upgrade-path-31.patch307.18 KBmradcliffe
#31 interdiff.txt1.29 KBmradcliffe
#29 2049465-image_style-upgrade-29.patch307.18 KBmradcliffe
#27 2049465-image_style-upgrade-27-FAIL.patch305.69 KBandypost
#27 2049465-image_style-upgrade-27.patch307.19 KBandypost
#23 2049465-image_style-upgrade-23.patch307.19 KBandypost
#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
#17 2049465-image_style-upgrade-17.patch1.69 KBniko-
#17 2049465-image_style-upgrade-test-17.patch2.9 KBniko-
#15 2049465-image_style-upgrade-15.patch4.56 KBandypost
#14 image.2049465.Upgrade_of_image_styles_and_effects_broken_fix.14.d8.patch977 bytesniko-
#10 image.2049465.Upgrade_of_image_styles_and_effects_broken_fix.10.d8.patch955 bytesniko-
#1 image.2049465.empty_image_styles_labels_fix.1.d8.patch820 bytesniko-
экрана от 2013-07-24 18:18:16.png73.54 KBniko-
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

niko-’s picture

tstoeckler’s picture

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.

tstoeckler’s picture

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

niko-’s picture

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

tim.plunkett’s picture

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

niko-’s picture

thanks tim

niko-’s picture

andypost’s picture

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

andypost’s picture

Title: Empty image styles labels in admin after d7 to d8 upgrade » Upgrade of image styles and effects broken
niko-’s picture

image_ipdate_8000 patch attached

andypost’s picture

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

andypost’s picture

Status: Active » Needs review

let's see whats broken

Status: Needs review » Needs work
niko-’s picture

Status: Needs work » Active
FileSize
977 bytes

patch fix

andypost’s picture

Status: Active » Needs review
FileSize
4.56 KB

Proper fix for upgrade path and test

tim.plunkett’s picture

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

niko-’s picture

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

tim.plunkett’s picture

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

andypost’s picture

Status: Needs work » Needs review
FileSize
4.61 KB

#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

tim.plunkett’s picture

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

mradcliffe’s picture

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.

andypost’s picture

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.

andypost’s picture

Status: Needs work » Needs review
Issue tags: +D8 upgrade path, +Needs upgrade path tests
tim.plunkett’s picture

  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.

andypost’s picture

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.

mradcliffe’s picture

Status: Needs work » Needs review
FileSize
307.18 KB

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.

mradcliffe’s picture

Status: Needs work » Needs review
FileSize
1.29 KB
307.18 KB

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

mradcliffe’s picture

FileSize
15.87 KB

Attached correct interdiff.txt.

andypost’s picture

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

mradcliffe’s picture

Oh, I understand now. That comes from #2066993: Use \Drupal consistently in tests. Sorry.

mradcliffe’s picture

Issue summary: View changes

Add issue summary update.

mradcliffe’s picture

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

andypost’s picture

andypost’s picture

Status: Needs review » Reviewed & tested by the community

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

tim.plunkett’s picture

Status: Reviewed & tested by the community » Needs review

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

webchick’s picture

Issue tags: +beta blocker

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

jibran’s picture

LinL’s picture

Patch no longer applied, here's a reroll.

claudiu.cristea’s picture

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.

claudiu.cristea’s picture

Assigned: Unassigned » claudiu.cristea
Status: Needs work » Needs review
FileSize
514 bytes
309.22 KB
28.18 KB
74.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.

claudiu.cristea’s picture

Status: Needs work » Needs review

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

claudiu.cristea’s picture

Ups! The patch ;)

claudiu.cristea’s picture

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.

claudiu.cristea’s picture

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

claudiu.cristea’s picture

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.

claudiu.cristea’s picture

Status: Needs work » Needs review

This is ready for RTBC or more reviews.

andypost’s picture

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.

andypost’s picture

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Awesome work on this. Thanks so much!!

Committed and pushed to 8.x.

claudiu.cristea’s picture

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

Great! Untagging and removed myself.

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

Anonymous’s picture

Issue summary: View changes

Update from andypost's comments.

klonos’s picture