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

Cleaning up tags.

Can someone add an issue summary, please?

This is going to be tough to fix because some code, e.g. D7NodeDeriver intentionally calls deprecated code (for backwards compatibility).

Rerolled.

This gets a passing test, but hides a bunch of technical debt.

I suspect that the migrate_cckfield_plugin_manager_test test module will need some more updating, but I'm not familiar enough with the migrate system to know what to snip.

We cannot change the annotations on the deprecated field plugins. We still need to fix all over the place so this is fixed more properly.

OK, discussed this a little bit more in the migrate maintainers meeting with @mikelutz, @phenaproxima, @quieteone, @heddn. I wanted to get those notes added for a little more clarification.

I don't think there is really a way to get rid of /these/ deprecations. Because we have two plugin managers. All of core uses the new plugins with a BC shim for contrib. And we also have a BC layer for anyone who still has existing migrations using the old cck plugins. And those legacy plugins will get discovered by plugin discovery. When we get ready for 9.0... we'll just remove the legacy cck plugins and the plugin manager. And there will be much rejoicing. It will be easy and fun.

The goal isn't to get rid of deprecations, but to get rid of technical debt related to deprecations. :-)

In this case I think it's just that we need to mark some tests as legacy and that's it.

If those plugins are in a quasi-non-deprecated state then there can be a follow-up about whether to leave them like they are or not. We can make passing tests that don't ignore their deprecation status now, and that will aid in the follow-up.

These deprecations happened in #2683435: CCK does not exist in Drupal 7, rename Migrate's cckfield plugins to field plugins

This patch reverts the annotation changes and then adds @group legacy and expected deprecations to tests. I manually tested migrate and migrate_drupal modules, so there might be some stragglers to get on the next patch.

Picking up the fails. Let's see what I missed.

Let's see if this gets them all.

So, once consideration with the last few patches is that once we drop the cck plugins in 9.0, we'll have to somehow figure out what tests need the @legacy removed from. The tests aren't deprecated or legacy. But due to plugin discovery, basically everything that deals with fields is going to trigger the deprecation warning. This seems like more work than needed, especially since it will only lead to more work later.

You know, It's not so much that the tests need to be marked legacy. A lot of this is because these tests, which load up the node migrations and run through the derivers which try to find the field plugins fail because we don't load the actual field modules with the FieldPlugins, so field plugin lookup fails, then cck lookup fails, then you default to a basic get process.

There might be some work involved, but if the tests are set up right, I think all the core fields we need to migrate have a field plugin, so if that's found, the cckplugin deprecation wouldn't get triggered. This might be an issue of making sure all the tests have valid field plugins available.

And, if this works and we prevent tests from running the BC shim code (excepting the ones we have specifically to test it, which SHOULD be marked @legacy), Then we can probably get rid of the deprecation message on the CckPluginManager by not injecting the service into the derivers, but only creating it in the BC shim if needed.

It's looking like we might need to create a few more non-existent field plugins for d6 for this to work, but it still might be the 'right' way

Adding a related issue

Re: #22: If you remove MigrateCckField, then it won't trigger with its specific deprecation error message.

Since these tests all have an @expectedDeprecation annotation for that message, they will fail if it's not present.

As to whether it's a lot of work, of course it is. :-) That's how technical debt works... We're also going to have to do the same thing as this issue for MigrateCckFieldPluginManager and MigrateCckFieldPluginManagerInterface deprecation errors. I thought about folding those messages in here, too, but the patch in #20 is already 140k.

Right, but like @heddn said, we don't want to add the expected deprecation message to all those tests. That adds more debt than it takes away. we need to refactor the code so that core tests don't trigger these errors or we leave the deprecation message globally suppressed until Drupal 9 is open and we take out the BC shims and everything is fine.

The issue I attached needs more work, but it would take all the BC shim code and move it to one place, making the ultimate removal easier.

Also, I may be running down a rabbit hole here, but I'm going to try a few things to see if I can fix the tests such that they aren't calling that deprecated service.

Right, but like @heddn said, we don't want to add the expected deprecation message to all those tests.

I'm not seeing where @heddn said that.

we need to refactor the code so that core tests don't trigger these errors

That's out of scope here. See #16... Make a follow-up, and/or postpone this issue on that one.

or we leave the deprecation message globally suppressed until Drupal 9 is open and we take out the BC shims and everything is fine.

If there's a plan issue about this, then let's link it and postpone here.

Postponed on #3003920: Plan how to expose deprecated migration entities

Just wanted to throw this against the testbot.

So, this approach is valid, though it needs polish. We needed two new field plugins, one for d6 phone and one for d6 number. The user field discovery needed to be converted to the one used by the node and taxonomy derivers, where they try to load the field plugin by the type map. Looking at it, this is buggy anyway, since calling FieldPluginManager::hasDefinition($field_type) only looks for plugins that have the field type as the id, which is wrong. This issue would be fixed in #2951550: Make service for field discovery for use in migrate entity derivers which would consolidate all that code. We need to add the field modules to the base kernel test in migrate drupal so the actual Field Plugins can be found and we don't fall back to trying to find the CCK plugins, and this creates an upgrade path for d6 phone.

Really what's left here is to write tests for the two new field plugins, and test/finish the upgrade path for d6 phone.

I think the best bet is to postpone this on a couple issues. First, we should finish #2951550: Make service for field discovery for use in migrate entity derivers to consolidate the code that calls CckPluginManager, and then we should create the upgrade path for d6 phone probably in a separate issue.

Adding the d6 number field plugin I think negates the need for some of the field mapping in the d6_field migration, which should be looked at.

I'm moving this back to review just so it's on the radar for the maintainers meeting, but I think it does end up needing to be postponed on a couple other issues.

Only a very brief look at the patch and spotted this.

+++ b/core/modules/migrate_drupal/tests/src/Kernel/MigrateDrupalTestBase.php
@@ -15,7 +15,21 @@
+  public static $modules = [

Enabling 7 more modules here will cause problems for anyone extending MigrateDrupalTestBase in contrib. I know that this has had modules enabled since the beginning but we really need to stop adding to it (and yes, I have added to it in the past).

Ideally, I think, MigrateDrupalTestBase should not install any modules.

Well, the alternative is to enable them in all the classes that extend this one, I suppose. On its own that only fixed about 20 tests, though I suspect most of the other 113 would still be broken without enabling them. Anything running Drupal migration tests against the fixtures in core really should have all those modules installed. Just running the node derivers requires all of those field plugins to be discovered to avoid calling the deprecated cck plugin manager.

I'm not sure I completely understand why that would cause problems in contrib. I suppose if you wrote your test expecting one of those modules not to be enabled the results could change, but I didn't think the test classes were part of the API, or required to be backwards compatible. I don't want to cause problems in contrib, I just don't understand the issue.

I guess I should have expanded on why I postponed this in #30...

This issue addresses one deprecation error that's ignored by the deprecation reporting system. There are quite a few others... In fact it's fair to say that most of the remaining ones are related to the migration system.

So since @heddn was talking about a plan to remove these classes before D9, I made a *plan* issue so we could be sure this happened at some point in the future.

Changing the legacy migration plugin deprecations isn't in scope here (see #15), so maybe go over to the plan issue to figure it out.

#3003920: Plan how to expose deprecated migration entities

I think you are misunderstanding my patch. The scope of this issue and its parent meta is to make sure that core does not call deprecated code, with the goal of removing items from the global deprecation ignore list. I discussed this with the migrate maintainers last Thursday (#15) where the initial thought was that we had to call this code as a BC shim to support contrib modules which may be using the deprecated plugin annotations to define fields. I agreed to look a little further into the issue during that meeting to see if there were any options to ensure that core does not call that deprecated code unless there is an actual contrib module that is using it. What I found was that it actually is possible to ensure that these notices aren't triggered just from running core field migrations.

What we don't want to do is mark a bunch of tests as legacy when they aren't. The tests are valid and shouldn't be removed when Drupal 9 starts, so marking them legacy and adding deprecation notices now would require us to go through and remove them later, and doesn't actually help make sure the deprecated code is isolated.

We are not changing deprecations or removing them with this patch. Core-only migrations don't use the deprecated plugins, but due to some missing dependencies and missing Field Plugins core still calls up the deprecated plugin manager for core-only migrations, and it shouldn't. I have marked this as needs review because I would like it on the list when we discuss outstanding migration issues in tomorrow's meeting, so I can report what I found, and the possible solution to remove the deprecation suppression for this issue. I promise the status will be changed during the meeting one way or another, as the maintainers decide what to do.

You are correct in that 10 of the 40 deprecation suppressions belong to the migration system, and I've responded to your other planning issue as well. It looks like many of them don't need to be there, and a couple others are fairly easily fixed in core. I agree we absolutely need a plan to remove the ones we can and determine if there are any that we truly can't remove right now, and make sure we have a future plan in place for how to address removing the deprecated code in the future.

mikelutz just talked to me on Slack about this, which was a good thing to do. I remove my concerns in #33. I was thinking that change was in the MigrateUpgrade tests which it is not. So that part of the patch that caught my eye is really fine.

Postponing on #2951550: Make service for field discovery for use in migrate entity derivers

Looks like this can be unpostponed, that other issue is closed?

Just a reroll of the previous patch. Was relatively easy by applying the patch to a drupal core version of ctober 2018 and then rebasing, only one conflicting chunk then in the User migration plugin. I just dropped the customization we have here for now, lets see what happens.

> Ideally, I think, MigrateDrupalTestBase should not install any modules.

Kernel tests need a lot of explicit dependencies, so I'm not sure that is a good goal. But adding them will at least make the tests slower, as we're installing at least some of these entity schemas all the time.

I also had a look at the MigrateCckFieldPluginManager related deprecation messages, and the thing is that you simply can't do that. You can't put a @trigger_error() into a class that's defined as a service, same as #3009854: Fix "The "serializer.normalizer.file_entity.hal" normalizer service is deprecated: it is obsolete, it only remains available for backwards compatibility." deprecation error, this is automatically called by drupal as part of the plugin cache clear service, to check that the definition is correct and so on. I tried moving it to the constructor, but that didn't work either, because again, the cache clear mechanism we have for plugin managers call it automatically on module installations and that can't be avoided. So I'm just removing it, we still have @deprecated so things like drupal-check would see any usage. It's more important that we detect the actual plugins.

Interesting, the service deprecation didn't happen in the the test that I did run, but that likely needs to be removed for the same reason.

Reroll and removing the deprecated from the service definition.

Actually removed it now ;)

Starting to think we can't actually get rid of this message and might have to keep ignoring that until we actually remove it in 9.x?

So here's a version that keeps the service deprecation message exclusion, if we have to keep one, that seems more honest ;) Interdiff against #45.

My thoughts in no particular order:

• trigger_error should be removed from the plugin manager service class, it should be added to the createInstance and getPlugin fromFieldType methods.
• We should have a way to deprecate plugin manager services, It probably means we need to change the error handler on cache-clears and trigger the error from the container.
• The new field plugins should be broken into their own issue probably and this should be postponed on it. Either way, they need explicit tests too.

Tagging as a requirement for Drupal 9.0-beta1.

Working on a reroll.

core/tests/Drupal/Tests/Listeners/DeprecationListenerTrait.php needed a reroll.

Bumping priority since this needs to be resolved one way or the other prior to 9.0.0's release.

Going to work on the failing test.

FYI Drupal\Tests\migrate_drupal\Kernel\d6\FieldDiscoveryTest is the test that fails in #55.

It seems like it just needed some extra lines in addAllFieldProcessesAltersData() to indicate the new items.

+++ b/core/lib/Drupal/Core/Field/Plugin/migrate/field/d6/NumberField.php
index 0000000000..1e803f1c4c
--- /dev/null

--- /dev/null
+++ b/core/lib/Drupal/Core/File/Event/FileEvents.php

+++ b/core/lib/Drupal/Core/File/Event/FileEvents.php
index 0000000000..264d7ab995
--- /dev/null

--- /dev/null
+++ b/core/lib/Drupal/Core/File/Event/FileUploadSanitizeNameEvent.php

This looks like it is not part of this patch.

This is really completely unnecessary. We've already removed the deprecation message suppression and everything else CCK related from Drupal 9 in #3088480: Remove the "cckfield" plugin type from the migration system. I think there is little to no value in doing this to 8.9.x, we are only further diverging the codebase from 9.0.x. I'm not sure why this ended up as a 9.0 beta requirement as it only applies to 8.9. I'm untagging and closing this.