Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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.
Comment | File | Size | Author |
---|---|---|---|
#60 | 55-60-interdiff.txt | 3.62 KB | alexpott |
#60 | 1464554_60.drupal8.image-style-upgrade.patch | 7.39 KB | alexpott |
#55 | 52-55-interdiff.txt | 4.24 KB | alexpott |
#55 | 1464554_55..drupal8.image-style-upgrade.patch | 7.63 KB | alexpott |
#52 | 1464554_52.drupal8.image-style-upgrade.patch | 6.49 KB | alexpott |
Comments
Comment #1
cosmicdreams CreditAttribution: cosmicdreams commentedThis 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.
Comment #2
catchComment #3
gddtagging
Comment #4
chx CreditAttribution: chx commentedComment #5
jhedstromThis 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.
Comment #6
jhedstromErr, the actual patch.
Comment #7
xjmSending to the bot.
Comment #8
jhedstromSetting back to needs work since the patch in #6 is just a start. Going to try to push this further today.
Comment #9
gddI have an in-progress patch on this I can post in about an hour or so here, ust need to finish a call.
Comment #10
gddSo... 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.
Comment #11
jhedstromDid 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.
Comment #12
jhedstromThis 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.
Comment #14
gddThe 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.
Comment #15
gddThere 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.
Comment #16
andypostUpgrade path for Image styles should copy style's name to label if #606598-96: Human readable image-style names gets commited
Comment #17
cosmicdreams CreditAttribution: cosmicdreams commentedShould 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.
Comment #18
cosmicdreams CreditAttribution: cosmicdreams commentedComment #19
sunI'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.
Comment #20
cosmicdreams CreditAttribution: cosmicdreams commentedOK. going to work on that tonight.
Comment #21
aspilicious CreditAttribution: aspilicious commentedWhoaha! Took more time than expected. Lots of patch comparing. Only this part:
Needs some work, it throws a notice :) But all the tests seem to work. Nice!
Comment #22
aspilicious CreditAttribution: aspilicious commentedLooking 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...
Comment #24
aspilicious CreditAttribution: aspilicious commentedThis works locally :)
Comment #25
aspilicious CreditAttribution: aspilicious commentedComment #26
aspilicious CreditAttribution: aspilicious commentedIt'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
Comment #27
Ogredude CreditAttribution: Ogredude commentedWith 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.
Comment #28
disasm CreditAttribution: disasm commentedAfter 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.
Comment #30
disasm CreditAttribution: disasm commentedAttached is patch 24 with update hook incremented. Ignore the above one, I merged the two patches accidentally.
Comment #32
aspilicious CreditAttribution: aspilicious commentedWTF
Why? is this failing here, isn't related at all. And why is the procedural function looking inside the Drupal\system\Tests\Upgrade\ direcotry...
Comment #33
tim.plunkett#30: drupal-system_image_upgrade_path-1464554-29.patch queued for re-testing.
Comment #35
tim.plunkettWell, 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?
Comment #36
tim.plunkettEr, 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
Comment #37
aspilicious CreditAttribution: aspilicious commentedHmm, very veyr strange, lets see what happens with these patches...
Comment #38
aspilicious CreditAttribution: aspilicious commenteddamnit...
Comment #40
aspilicious CreditAttribution: aspilicious commentedComment #42
aspilicious CreditAttribution: aspilicious commentedWhops wrong patch
Comment #43
aspilicious CreditAttribution: aspilicious commentedFinally green again, stupid test bots...
Based on #27
And #28
This is RTBC. But this can use a code review.
Comment #44
sun788e22f Various fixes and clean-ups.
Comment #45
sunComment #47
sunb59435f Dear testing framework, please DIE.
Comment #49
sunThe image_desaturate effect is missing for some reason.
Comment #50
gddNote 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.
Comment #51
sun#1508872: Image effects duplicate on edit landed, let's get back to this! :)
Comment #52
alexpottPatch 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.
Comment #53
alexpottFixing title...
Comment #54
attiks CreditAttribution: attiks commentedwe're using uuid for this:
If there a reason not use use
image_effect_save
?Comment #55
alexpottGood 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 patchComment #56
gddIs 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.
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.
Comment #57
gdd#55: 1464554_55..drupal8.image-style-upgrade.patch queued for re-testing.
Comment #59
gddYeah 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.
Comment #60
alexpottThe attached patch implements suggestions from #56 and uses a more reliable way to map uuids to effects for the upgrade test.
Comment #61
gddShip that thing!
Comment #62
Dries CreditAttribution: Dries commentedLooks good to me, and comes with tests. Committed to 8.x.
Comment #63.0
(not verified) CreditAttribution: commentedUpdated issue summary to remove system site.performance stuff.