In attempting to reduce the list of ignored deprecation errors in #2959269: [meta] Core should not trigger deprecated code except in tests and during updates ...
We get to an implementation issue here: #2970108: Fix "MigrateCckField is deprecated in Drupal 8.3.x and will be removed before Drupal 9.0.x. Use \Drupal\migrate_drupal\Annotation\MigrateField instead." deprecation error
Currently, Drupal core ignores many deprecation error messages from the migration system. Here is the list of messages related to the migration system:
In #2970108-15: Fix "MigrateCckField is deprecated in Drupal 8.3.x and will be removed before Drupal 9.0.x. Use \Drupal\migrate_drupal\Annotation\MigrateField instead." deprecation error @heddn points out that the plan seems to be to leave these ignored deprecations in place until very close to the Drupal 9 release.
Figure out what to do.
File implementation issues.
Update #2959269: [meta] Core should not trigger deprecated code except in tests and during updates with plans for how to remove these deprecation messages from the ignore list.
| Comment | File | Size | Author |
|---|---|---|---|
| #6 | 3003920-6.drupal.DateFieldDeprecation.patch | 2.46 KB | mikelutz |
| #4 | 3003920-4.drupal.MigrationProcess.patch | 4.11 KB | mikelutz |
| #3 | 3003920-3.drupal.i18nVariable.patch | 1.69 KB | mikelutz |
| #3 | 3003920-3.drupal.CommentVariablePerCommentType.patch | 1.69 KB | mikelutz |
| #3 | 3003920-3.drupal.CommentType.patch | 1.65 KB | mikelutz |
Comments
Comment #2
mile23Comment #3
mikelutzThe CCKPluginManager and related deprecations are still called as a BC shim, but I actually think we can make sure they aren't called from core or core tests, and make sure the deprecated service is only instantiated in the case of a contrib module using it to migrate a contrib field. My latest patch in #2970108: Fix "MigrateCckField is deprecated in Drupal 8.3.x and will be removed before Drupal 9.0.x. Use \Drupal\migrate_drupal\Annotation\MigrateField instead." deprecation error was only about 6k and reduced the number of failing tests down to a very manageable number. Once that is working, we can eliminate the plugin manager and service from being called by not injecting the service as a dependency in the derivers, but only accessing it from Drupal::service inside the shim code which won't be called by core tests.
The remaining deprecations I suspect just need to be fixed in core, but I'm not certain how pervasive they are, so here's some patches to tell us how much work they each need....
Comment #4
mikelutzSo most of those are only triggered in one test, so they should probably not have been added to the global list in the first place, or they've been fixed in core in other issues already, but not removed from the list. The remaining test for each tests the deprecated code, so they should have the legacy group tag and the expected deprecations. I know the migration plugin is still used in a few places in core that touch a lot of tests, so I don't think that one is a big deal either.
I haven't checked the datetime one yet.
I think I figured out what we need to do for #2970108: Fix "MigrateCckField is deprecated in Drupal 8.3.x and will be removed before Drupal 9.0.x. Use \Drupal\migrate_drupal\Annotation\MigrateField instead." deprecation error in #31 #2970108-31: Fix "MigrateCckField is deprecated in Drupal 8.3.x and will be removed before Drupal 9.0.x. Use \Drupal\migrate_drupal\Annotation\MigrateField instead." deprecation error.
If we can make that solution work then that means we don't actually trigger any CCK plugins in core anymore, and we don't go down the BC shim codepath anymore. Once that's true, we can remove the CCK plugin manager from dependency injection, and just call it from \Drupal::service() inside the code shim, where we already know it won't be executed in any tests not specifically testing that code shim (which should be legacy and be expecting the deprecation anyway). This should let us remove all the migration deprecations from the global list and put all the deprecated code in the easily removed code shims. This should be the right thing to do, it proves the code in core works without triggering the shims, which proves we can safely remove it when 9.0 starts.
Comment #5
heddnI am hoping to disarm the start of disagreements here. In fact, I think we are all arguing the same result. Especially in #2970108: Fix "MigrateCckField is deprecated in Drupal 8.3.x and will be removed before Drupal 9.0.x. Use \Drupal\migrate_drupal\Annotation\MigrateField instead." deprecation error. Let's see what we find over there with some small tweaks to how we are using/calling deprecated code within the migration system. Then we can use those findings in all the follow-up issues we have linked in here.
I'm much more inclined to see a 4k patch that removes some of our legacy calls from non-deprecated code than a larger patch that doesn't remove these legacy calls and results in a mundo patch. Can we all agree on that?
Comment #6
mikelutzComment #7
mikelutzAfter discussing with @heddn in slack, we'll do one issue to remove the global notices for i18Variable, CommentType, CommentVariable, CommentVariablePerCommentType. Those shouldn't be on the global list at all. They each have one or two tests that trigger them, and the tests are specifically to test the deprecated class, so those need to be marked legacy and it's done.
We'll create issues for fixing migration and date field. We already have #2970108: Fix "MigrateCckField is deprecated in Drupal 8.3.x and will be removed before Drupal 9.0.x. Use \Drupal\migrate_drupal\Annotation\MigrateField instead." deprecation error for the CCK Plugin, and the remaining three all deal with the CCK Plugin Manager, so those can be done at the same time after 2970108 is fixed.
Comment #8
mikelutzComment #12
quietone commentedThanks to Mile23 for creating this issue so this could be competed.
Discussed this a the migrate meeting today and mikelutz says we are passed this and it is outdated.