Follow-up to #2625696: Make migrations themselves plugins instead of config entities

Problem/Motivation

MigrationCreationTrait used to create config entities for migration templates. It no longer does. It configures the plugins.

Proposed resolution

Rename it.

Remaining tasks

rename MigrationCreationTrait to MigrationConfigurationTrait to reflect its current behavior and any associated comments

User interface changes

API changes

Data model changes

Comments

alexpott created an issue. See original summary.

mikeryan’s picture

mikeryan’s picture

Issue tags: +Novice
mikeryan’s picture

Version: 8.1.x-dev » 8.2.x-dev
willwh’s picture

FileSize
2.25 KB

Yanking this for sprint mentoring to let someone else have a crack :)

willwh’s picture

barbarae’s picture

Issue summary: View changes
mikeryan’s picture

Status: Active » Needs review
hussainweb’s picture

It doesn't look like that the patch in #5 would run, but I think it would fail anyway (the file was not renamed). I also updated the comment and removed the todo. I am uploading an updated patch.

douggreen’s picture

hanoii’s picture

Looking at this at Drupal NOLA sprint, we are looking into getting it ready for RTBC.

hanoii’s picture

We were reviewing this issue with @heddn, it looks simple enough to be RTBC.

I did a project-wise search making sure there's no any leftover references to the old name, no doc comment or anything.

hanoii’s picture

Status: Needs review » Reviewed & tested by the community
alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Can we leave the old trait around - but it should just use the new trait. And deprecated it for 8.3 - this means that people using the migrate upgrade module (from contrib) don't have to do a version dance.

hanoii’s picture

Status: Needs work » Needs review
FileSize
14.49 KB

Attempting what @alexpott suggested.

alexpott’s picture

Status: Needs review » Needs work
FileSize
12.52 KB

Here's an interdiff - For instructions on creating an interdiff, see https://drupal.org/documentation/git/interdiff

+++ b/core/modules/migrate_drupal/src/MigrationCreationTrait.php
@@ -2,172 +2,15 @@
+ * @todo: To be deprecated in 8.3 so not to break contrib modules using this
+ * trait.

There's a specific format for @deprecated notices... for example:

 * @deprecated in Drupal 8.0.x and will be removed in Drupal 9.0.x. Use
 *   \Drupal\views\Plugin\views\display\EntityReference instead.

The last submitted patch, 15: rename-2682585-15.patch, failed testing.

mikeryan’s picture

Version: 8.2.x-dev » 8.1.x-dev

I think, since we are maintaining the old trait, the change is upward-compatible and can go into 8.1.x.

Not sure if the test fail is real or an instance of #2724871: Random failure in \Drupal\migrate_drupal_ui\Tests\d7\MigrateUpgrade7Test, let's retest.

The last submitted patch, 15: rename-2682585-15.patch, failed testing.

hanoii’s picture

Status: Needs work » Needs review

seems to be passing except from one older one.

heddn’s picture

Status: Needs review » Needs work

The last submitted patch, 21: rename-2682585-21.patch, failed testing.

heddn’s picture

Status: Needs work » Needs review
lokapujya’s picture

Running the 8.1 test again to see how consistently this issue reproduces it. Looks like it passed the 8.2 tests.

Status: Needs review » Needs work

The last submitted patch, 21: rename-2682585-21.patch, failed testing.

heddn’s picture

Status: Needs work » Needs review
alexpott’s picture

Status: Needs review » Needs work
FILE: ...t.dev/core/modules/migrate_drupal/src/MigrationCreationTrait.php
----------------------------------------------------------------------
FOUND 1 ERROR AND 1 WARNING AFFECTING 2 LINES
----------------------------------------------------------------------
  5 | WARNING | [x] Unused use statement
 13 | ERROR   | [x] The closing brace for the trait must have an
    |         |     empty line before it
----------------------------------------------------------------------
PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Time: 27ms; Memory: 3.5Mb
heddn’s picture

Status: Needs work » Needs review
FileSize
14.65 KB
14.65 KB
heddn’s picture

FileSize
636 bytes

I must really want that patch tested. Or I just goofed and upload the wrong thing for the interdiff.

dscl’s picture

Hey guys,

I went through the comments and noticed @alexpott suggested to mark it as deprecated to be removed on 8.3.x (#14), but the patch is saying it will be remove on 9.0.x.
I believe the confusion happened because @alexpott provided an example (#16) where 9.0.x was mentioned.

Should we change it to 8.3.x or is keeping 9.0.x also fine?

dscl’s picture

Just in case, I'm submitting a new patch changing it to 8.3.x as I've mentioned before.
If 9.0.x is fine, just ignore it.

heddn’s picture

I don't think we tend to designate a specific point release when something is going to go away. It is too hard to forecast the future. But that does bring up a good point. Typically we are more clear about stating "before".

New patch attached that copies more exactly the typical pattern.

Mile23’s picture

Status: Needs review » Reviewed & tested by the community

So a) If you set up your git like this:

 [diff]
  renames = copies

...it's a lot easier to read the patch and see what's actually changed. https://www.drupal.org/documentation/git/configure

b) I was curious what changed in the new file from the old one, so I did this:

$ git checkout -- core/modules/migrate_drupal/src/MigrationCreationTrait.php
$ diff core/modules/migrate_drupal/src/MigrationCreationTrait.php core/modules/migrate_drupal/src/MigrationConfigurationTrait.php 
11,14c11
<  * Creates the appropriate migrations for a given source Drupal database.
<  *
<  * @todo https://www.drupal.org/node/2682585 The trait no longer creates
<  *   migrations.
---
>  * Configures the appropriate migrations for a given source Drupal database.
16c13
< trait MigrationCreationTrait {
---
> trait MigrationConfigurationTrait {

So no code changes, just the appropriate name and comment changes.

As far as deprecation, yah, we should be specific about when the code will be removed, so 9.0.0 is most appropriate. And we should say that it's been removed in the version target for the issue, so 8.1.x is fine as long as we're targeting 8.1.x.

I think it might be that we need to bump up to 8.2.x for an API change, but a maintainer can make that call, referencing #18.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 32: rename-2682585-32.patch, failed testing.

quietone’s picture

Mile23’s picture

Status: Needs work » Reviewed & tested by the community

Passed this time.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 32: rename-2682585-32.patch, failed testing.

dscl’s picture

Status: Needs work » Reviewed & tested by the community

Passing now, so changing back to RTBC.

Sure it was the mentioned issue on random fails.
Now that issue is fixed and we shouldn't have this test failing again.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: -Migrate BC break

Committed 46434aa and pushed to 8.1.x and 8.2.x. Thanks!

I committed this 8.1.x because migrate is experimental and the change is totally BC.

  • alexpott committed f89e84d on 8.2.x
    Issue #2682585 by heddn, dscl, hanoii, willwh, hussainweb: Rename...

  • alexpott committed 46434aa on 8.1.x
    Issue #2682585 by heddn, dscl, hanoii, willwh, hussainweb: Rename...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.