Separate cache bin for migrations

The last patch had an extraneous line in the file, just fixed that. Patch looks good to me.

RTBC, I didn't actually write any of this patch.

Why not use a null cache backend with cache.discovery for this?

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

It'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.

Yeah 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?

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

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

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

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

For me personally a separate cache bin is totally enough + an entry in sites/example.settings.local.php

+++ b/core/modules/migrate/migrate.services.yml
@@ -17,6 +17,12 @@ services:
+      - { name: cache.bin, default_backend: cache.backend.memory }

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

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

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

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

In one of the random fail issues @heddn said that this change also caused #2736789: Default of 'migration' database key is ONLY thing that works

So one thing to try would be a patch that uses the memory backend for discovery and see if we get even more random failures.

Well, we have random failures, so let's try to help people with it, by still having a settings.local.php entry.

I was checking whether the patch needs a reroll, and it had some offset. Updated patch.

patching file core/modules/migrate/migrate.services.yml
Hunk #1 succeeded at 21 (offset 4 lines).
patching file sites/example.settings.local.php

Back to RTBC

+++ b/sites/example.settings.local.php
@@ -67,6 +67,16 @@
+ * While working on migrations it can be a hassle, as one has to clear caches
+ * all the time.
+ *
+ * By commenting out this bit, you never store migration caches.

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

Documentation improved as per comment #41, Also added interdiff.

Back to RTBC

Committed 87fdc92 and pushed to 8.3.x. Thanks!