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/Motivation
Per #2533886: [meta] Move module-specific migration support into the particular modules supported, move support for action migration into the action module.
According to usage statistics on the ImageCache project page 60,000 to 70,000 sites are still using the ImageCache module on D6 sites. This in connection with its inclusion into core for D7 heavily suggests that we should include a core migration package to handle these use cases.
Proposed resolution
Add migration to core
Remaining tasks
none
User interface changes
none
API changes
none
Data model changes
none
Comment | File | Size | Author |
---|---|---|---|
#34 | 2510160-34.interdiff.txt | 5.81 KB | neclimdul |
#34 | d6_imagecache_d8-2510160-34.patch | 18.41 KB | neclimdul |
Comments
Comment #1
lostkangaroo CreditAttribution: lostkangaroo commentedThis could stand to take a good review as some of the base mechanics are still fuzzy to me, but this patch works. I would like to see the id mapping not be located in its own process plugin but included in the template if possible for easier extensibility.
Comment #2
lostkangaroo CreditAttribution: lostkangaroo commentedComment #5
phenaproximaThis is oddly structured because it's going out of its way to call parent::import(). As far as I know, it won't cause any problems if the style is built and saved in this method, without bothering to call parent::import(). As long as it returns the saved entity ID, things should continue to work. IMO this should be refactored, but if it needs to keep this structure then there should at least be comments explaining why.
Definitely not a big deal, but this could be made shorter by using preg_replace(). There's also no need to set $id to NULL in the default case, because empty() will do an implicit isset() check.
Use $this->select(), not $this->database->select(). $this->select() will set the proper fetch option for you. There's also no need to select all the fields manually -- you can just use
fields($table)
. The reason other source plugins do it the long way is because it's a relic of the Fake DB driver, which is now gone.Other than that, this looks pretty good to me.
Comment #6
lostkangaroo CreditAttribution: lostkangaroo commentedDid some renovation work to how this migration moves the data taking the feedback into accord.
1) Removed the parent::import method call. Also added a try catch to catch any PluginNotFoundExceptions that will occur if the ImageStyle effect does not have a corresponding plugin in D8 and throw a Migration Exception. This is the case in two options allowed in D6: imagecache_deprecated_scale, imagecache_sharpen.
2) Yeah hated the switch. Its primary need was to filter out the two effect cases stated above and set the d8 effect names but with the rewritten method it is no longer needed.
3) Thanks for the pointers on the query useage most of that was modified copy and paste from other relic sources.
Still needs tests but will hopefully get those in soon. Just looking to see if this new code passes the smell test better before continuing.
Comment #7
phenaproximaLooks a lot better. I like the use of exception handling to catch invalid plugins.
MigrateException should be fully-qualified, and please add an explanation about the circumstances under which the exception is thrown.
Total nitpick: no space between the method name and the parameters.
I don't recommend str_replace() here. Use preg_replace(), because you only want to change it if the action name starts with imagecache_. str_replace() will replace it anywhere it appears.
Comment #8
lostkangaroo CreditAttribution: lostkangaroo commentedRound 3, now to add tests.
Comment #9
lostkangaroo CreditAttribution: lostkangaroo commentedComment #10
lostkangaroo CreditAttribution: lostkangaroo commentedFirst pass at getting tests in place.
Comment #12
phenaproximaThe dump files listed in setUp() are missing the .php extension.
Comment #13
lostkangaroo CreditAttribution: lostkangaroo commentedAlways something simple in the first draft
Comment #15
lostkangaroo CreditAttribution: lostkangaroo commentedComment #16
lostkangaroo CreditAttribution: lostkangaroo commentedComment #17
lostkangaroo CreditAttribution: lostkangaroo commentedComment #18
neclimdulreroll. leaving nw.
Comment #19
lostkangaroo CreditAttribution: lostkangaroo commentedStill based on the current migrate_drupal and not ready to be committed. Remaining tasks include testing exceptions raised during migrations caused by missing effect plugins, and crop effects used in D6 that do not use keyword anchors.
Comment #20
lostkangaroo CreditAttribution: lostkangaroo commented@phenaproxima mentioned in IRC that updated variable and system tables might be beneficial.
Comment #21
lostkangaroo CreditAttribution: lostkangaroo commentedComment #22
lostkangaroo CreditAttribution: lostkangaroo commentedComment #23
lostkangaroo CreditAttribution: lostkangaroo commentedComment #24
lostkangaroo CreditAttribution: lostkangaroo commentedComment #25
lostkangaroo CreditAttribution: lostkangaroo commentedblocker is gone so no longer postponed
Comment #26
neclimdulupdated patch.
Fixes changes from #2499793: Several migrate_drupal migrations fatal error on count(). Since we don't run the migration in the setup there are failures. Also, MigrateTestBase doesn't clean up its variables in tear which leads to really useless errors if you aren't running the migration.
Gets the failure test working.
Comment #27
lostkangaroo CreditAttribution: lostkangaroo commentedComment #29
lostkangaroo CreditAttribution: lostkangaroo commentedMight want to remove this in the next pass.
Comment #30
neclimdulstill needs work i think but here's a reroll.
Addressed #29
Comment #31
neclimdulComment #32
lostkangaroo CreditAttribution: lostkangaroo commentedThis looks good and since no remaining tasks are present lets get this in.
Comment #33
phenaproximaI'm sorry to do this after hitting RTBC, but I found some things (mostly minor quibbles)...
Please remove the @throws line. IIRC, the API documentation generator doesn't like it if we mix @inheritdoc with other things.
If possible, please pass $e as the previous exception wrapped by the MigrateException, so that we don't lose the context in which the exception was originally thrown.
No @throws here.
Thar be an extra line between the doc block and the class.
It should be OK to just do
->fields('icp')
here.Please put this in the migrate_drupal_6 test group so that MigrateDrupal6Test will pick it up. The d6_imagecache_presets migration should also be added to its giant laundry list of migration IDs.
This pattern is the cause of our MigrateDrupal6Test woes. It's preferable to just execute the dependency directly: $this->executeMigration('d6_imagecache_presets')
Super-annoying code style nit: there should be a space between the comment text and the //. Also please use capitalization and punctuation, just for consistency with the rest of core.
Moar grammar policing. Should be "Test that missing actions cause failures."
This seems strange to me. What would be wrong with simply
$this->assertIdentical($effect_config['id'], $id)
and$this->assertEqual($effect_config['data'], $config)
?This needs to go in core/modules/image/config/schema/image.source.schema.yml.
Comment #34
neclimdulThink I got everything. Things that didn't change.
1) Didn't change group. I purposely excluded this from MigrateDrupal6Test
2) Didn't move the Migration run to the setup. The tests methods modify the source tables to assert different conditions so have to run the migration after that happens. This is the reason for #1.
3) assertImageEffect does take a bit to wrap your head around. The trick is the interaction of the return inside the loop. The loop is iterating looking for a matching effect. Non-matching effects are actually ok, the failure is that when it is unable to find the matching effect. Did comment to make this a bit clearer though.
Comment #35
phenaproximaGood enough for me.
Comment #36
webchickCommitted and pushed to 8.0.x. Thanks!