Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
Right now the only way to develop migrations is drush cr
after every file save. This is painful.
Proposed resolution
Separate out migrate plugin definitions into their own cache backend so that it can be cache.backend.null while in development.
Remaining tasks
User interface changes
API changes
Data model changes
Comment | File | Size | Author |
---|---|---|---|
#42 | interdiff-2694391-38-42.txt | 650 bytes | yogeshmpawar |
#42 | 2694391-42.patch | 1.51 KB | yogeshmpawar |
#38 | 2694391-38.patch | 1.53 KB | iMiksu |
#34 | 2694391-34.patch | 1.53 KB | dawehner |
#34 | interdiff.txt | 1.21 KB | dawehner |
Comments
Comment #2
chx CreditAttribution: chx at Smartsheet commentedComment #4
chx CreditAttribution: chx at Smartsheet commentedComment #5
benjy CreditAttribution: benjy at PreviousNext commentedThe last patch had an extraneous line in the file, just fixed that. Patch looks good to me.
Comment #6
benjy CreditAttribution: benjy at PreviousNext commentedRTBC, I didn't actually write any of this patch.
Comment #7
catchWhy not use a null cache backend with cache.discovery for this?
Comment #8
benjy CreditAttribution: benjy at PreviousNext commentedWhy would we needlessly disable all caching? I would have thought that its more likely the migrations haven't changed vs have in all scenarios apart from during development of a migration.
Also, it could slow down the UI that provides a list of migrations or other tools unless they provide their own caching.
Comment #9
catchIt's not all caching, it's cache.discovery. I don't see how disabling cache.discovery for migrations is any different to disabling for development of anything else that uses plugins.
Comment #10
benjy CreditAttribution: benjy at PreviousNext commentedYeah I meant cache discovery. If the UI does something like $manager->getDefinitions() to list all the migration plugins, won't that have to hit the filesystem every time the page loads?
Comment #11
chx CreditAttribution: chx at Smartsheet commented> Why not use a null cache backend with cache.discovery for this?
Because even in development that would be a very severe performance hit.
Comment #12
chx CreditAttribution: chx at Smartsheet, Integral Vision Ltd commentedHere's something even better: memory caching it. A (very common) use case is tinker -- rerun -- repeat -- yay works never touch it again. The UI can change back to the chained backend with adding a service provide w/ alter , it's really easy. There are no other pages where this hits.
Comment #13
chx CreditAttribution: chx at Smartsheet, Integral Vision Ltd commentedExcept I did wrong there and arguments need to be discovery_migration instead of discovery. Foolish me.
Comment #14
dawehnerAfter thinking about it for a while I think its fine to have its own entry for migrations. They are special in the way that unlike other plugins, you continuously edit the metadata of them.
There is no other plugin type where you have to deal with that all the time.
Comment #15
catchJust using memory all the time for this is a better justification for the separate cache service. Giving benjy another chance to look since he didn't like removing caching before.
Comment #16
alexpottGiven @catch's comment in #9 assigning to @catch. For my perspective this change makes sense. Creating migration plugins should never be a performance sensitive task and making life easier for developers seems sensible.
Comment #17
benjy CreditAttribution: benjy at PreviousNext commentedMy concern was that if you had a long running migration, like a large upgrade using batch, every new request would cause every migration to be rediscovered from disk? That might be a small performance hit for a one-off, but could it have a sizeable impact on long running / large upgrades.
Anyway, if everyone else here doesn't see it as an issue i've got no problem with this, I can change it back from the UI as needed anyway.
Comment #18
dawehnerFor me personally a separate cache bin is totally enough + an entry in
sites/example.settings.local.php
Comment #19
chx CreditAttribution: chx at Smartsheet, Integral Vision Ltd commentedI talked to benjy and pointed out that while rediscovery happens, reparsing not because file cache. This might or might not be enough. If #18 is what I can get then I will live with it of course however my experience is you spend much more time writing these things than running them also running them is slow enough and rediscovery won't be much worse however this will depend a lot on the system because if the system has enough memory for the filesystem to cache the files it'll be very fast on the other hand if you have the database on the same system and LRUs the plugin dirs out of FS cache that's bad but who runs on a single system where you have so much stuff these things matter
Comment #20
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedWhy set the default backend to memory?
I thought this was meant for local overriding in settings.local.php?
Is that not enough for developers to avoid the drush cr?
Not opposed to the change, but just wondering.
Comment #21
chx CreditAttribution: chx at Smartsheet, Integral Vision Ltd commentedWe might've crossposted? Read up one please.
Comment #22
dawehnerI agree with chx on this point. If your migration is slow because of the scanning of the definitions, you have kind of a luxury problem.
Comment #25
catchIf migrate was stable I'd want profiling here, since it's not I've gone ahead and committed to both 8.x branches.
I think we should profile a migrate batch request to see how bad this is, but that's worth doing in general and we might find other, worse things. Opened #2722287: Profile migrate UI batch request for that.
Comment #27
alexpottThis patch needs to be reverted. After much work I've found that it causes #2737599: Remote git bisect for random failure in \Drupal\migrate_drupal_ui\Tests\d7\MigrateUpgrade7Test. Once reverted we need to work out why on this issue.
Comment #30
alexpottIn one of the random fail issues @heddn said that this change also caused #2736789: Default of 'migration' database key is ONLY thing that works
Comment #31
catchSo one thing to try would be a patch that uses the memory backend for discovery and see if we get even more random failures.
Comment #34
dawehnerWell, we have random failures, so let's try to help people with it, by still having a settings.local.php entry.
Comment #35
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedComment #38
iMiksuI was checking whether the patch needs a reroll, and it had some offset. Updated patch.
Comment #39
iMiksuComment #40
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedBack to RTBC
Comment #41
alexpottThis documentation could be improved. Something like:
Uncomment the code below to only store migrations in memory and not in the database. This makes it easier to develop custom migrations.
Comment #42
yogeshmpawarDocumentation improved as per comment #41, Also added interdiff.
Comment #43
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedBack to RTBC
Comment #44
alexpottCommitted 87fdc92 and pushed to 8.3.x. Thanks!