Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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
Comment | File | Size | Author |
---|---|---|---|
#48 | drupal8.image-system.2049465-48-FAIL.patch | 306.82 KB | claudiu.cristea |
#48 | drupal8.image-system.2049465-48.patch | 313.08 KB | claudiu.cristea |
#48 | interdiff-against-46.patch | 5.52 KB | claudiu.cristea |
#48 | styles-d8-good.png | 42.65 KB | claudiu.cristea |
#46 | drupal8.image-system.2049465-45.patch | 307.91 KB | claudiu.cristea |
Comments
Comment #1
niko- CreditAttribution: niko- commentedPatch attached
Comment #2
tstoecklerIf this is in fact the case, it is certainly critical. It also means we're missing upgrade path tests in for this.
Comment #3
tstoecklerForgot the most important part: Thanks @niko- for testing the upgrade path so thoroughly and reporting this issue so early in the cycle. Awesomesauce!!!
Comment #4
niko- CreditAttribution: niko- commentedThanks @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
Comment #5
tim.plunkettThis should probably be combined with image_update_8000(). We can still do that until beta1.
Comment #6
niko- CreditAttribution: niko- commentedthanks tim
Comment #7
niko- CreditAttribution: niko- commentedcomplex patch attached to https://drupal.org/node/2049915#comment-7684707
Comment #8
andypostD7 image style labels landed #606598: Human readable image-style names so probably upgrade path should be changed accordingly
Comment #9
andypostRe-title and marked as duplicate #2049915: Missing image style effects after d7 to d8 migrate
Comment #10
niko- CreditAttribution: niko- commentedimage_ipdate_8000 patch attached
Comment #11
andypostcore/modules/system/lib/Drupal/system/Tests/Upgrade/ImageUpgradePathTest.php
should be fixed accordinglyComment #12
andypostlet's see whats broken
Comment #14
niko- CreditAttribution: niko- commentedpatch fix
Comment #15
andypostProper fix for upgrade path and test
Comment #16
tim.plunkettLet'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!
Comment #17
niko- CreditAttribution: niko- commentedsplited patches with move the config.factory outside the foreach.
Comment #18
tim.plunkett@niko-, that's backwards, that's just the fix. We need just the test.
Comment #20
andypost#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
Comment #21
tim.plunkettThis happened
Closed #2062341: Fix image upgrade path to work with image-style name backport as a dupe
Comment #22
mradcliffeThanks, 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 ofconfig_factory
to load the configuration entity.Comment #23
andypostAnd 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
anddrupal-7.bare.standard_all.database.php.gz
(added label field)So closed #606598-216: Human readable image-style names
Comment #25
andypost#23: 2049465-image_style-upgrade-23.patch queued for re-testing.
Comment #26
tim.plunkettThis makes the patch harder to read. Can you please leave the coding style alone and just change what is needed?
Same here
Also, please post one with just tests to prove the upgrade path tests work.
Comment #27
andypostThe 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
I see no reason get() each property
Comment #29
mradcliffeRe-roll patch. I ran UpdateLocaleTest, but it did not return any failures locally.
Note that the following is different from patch #27:
Comment #31
mradcliffeWell, 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
Comment #32
mradcliffeAttached correct interdiff.txt.
Comment #33
andypostJust a small nitpick - test should use own container
please revert this back to $this->container
Comment #34
mradcliffeOh, I understand now. That comes from #2066993: Use \Drupal consistently in tests. Sorry.
Comment #34.0
mradcliffeAdd issue summary update.
Comment #35
mradcliffeRevert change in #31 to #27 per meta issue.
Comment #36
andypost#27: 2049465-image_style-upgrade-27.patch queued for re-testing.
Comment #37
andypost#17 and #27 shows that test is valid, also tested manually on live D7 site
Comment #38
tim.plunkettYou can't RTBC this, it's half your code.
Comment #39
webchickTagging all critical D8 upgrade path issues as "beta blocker."
Comment #40
jibran#35: drupal-2049465-image-upgrade-path-35.patch queued for re-testing.
Comment #41
LinL CreditAttribution: LinL commentedPatch no longer applied, here's a reroll.
Comment #42
claudiu.cristeaI tried to manually test this issue. When running update.php the process breaks with:
Indeed, I see no
use Drupal\Component\Uuid\Uuid
inimage.install
header. But, anyway, it seems we have a brand new Uuid service since #1969572: Make Uuid a service.Comment #43
claudiu.cristeaThis one passes manual test.
But I see another problem. Styles shipped with Drupal are missed in D8.
D7 styles
D8 styles
And we need them!
Setting to "needs review" only to see tests. Taking the issue just to keep track.
Comment #45
claudiu.cristeaI posted a wrong patch in #43. Interdiff and all other remain.
Comment #46
claudiu.cristeaUps! The patch ;)
Comment #47
claudiu.cristeaAnd here is the explanation: Styles shipped with D7 (defined in
image_image_default_styles()
) are not filled in database (tablesimage_styles
andimage_effects
) until they are not saved for the first time. The upgrade path is scanning only theimage_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.Comment #48
claudiu.cristeaD7 styles
D8 imported styles
Comment #50
claudiu.cristeaPoorly named the interdiff. Setting back to "needs review".
Comment #52
claudiu.cristeaThis is ready for RTBC or more reviews.
Comment #53
andypostI think this rtbc! Anyway we can't access d7 styles that shiped with features or custom module because hooks are not allowed in update.
Comment #54
andypostThe change notice updated https://drupal.org/node/1820974/revisions/view/2411700/2855509
Comment #55
webchickAwesome work on this. Thanks so much!!
Committed and pushed to 8.x.
Comment #56
claudiu.cristeaGreat! Untagging and removed myself.
Comment #57.0
(not verified) CreditAttribution: commentedUpdate from andypost's comments.
Comment #58
klonos