The Migrate API uses configuration entities to store the configuration for each migration. The recent patch #2213451: Update Migrate API in Core adds the ability for migrations to depend on each other. The migrate API implements hard and soft dependencies between migrations. The dependency information is stored in a public property called dependencies

#2080823: Create API to discover config entities' soft dependencies and use this to present a confirm form on module uninstall adds a protected property to ConfigEntityBase called dependencies.

Obviously this is a conflict.

Preferred solution (for now) is to rename the migration entity's dependencies property to migration_dependencies. This is due to the large amount of work that has occurred in both patches AND the differing requirements since migrations only depend on other migrations and are never changed by a user through the UI where (proper) configuration entity dependencies are on modules, themes and other entities. A further reason a rename is preferable is the migrate's dependency management has no test coverage at the moment (afaics).

CommentFileSizeAuthor
#1 2221767.1.patch2.71 KBalexpott

Comments

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Component: configuration entity system » migration system
Status: Active » Needs review
StatusFileSize
new 2221767.1.patch2.71 KB

Status: Needs review » Needs work

The last submitted patch, 1: 2221767.1.patch, failed testing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs work » Needs review

Drupal\system\Tests\Common\RenderTest 137 6 0
Failure??? 1: 2221767.1.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 1: 2221767.1.patch, failed testing.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented 
Segmentation fault
FATAL Drupal\views\Tests\Handler\AreaTest: test runner returned a non-zero error code (139).

Noooo......

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

An alternative would be to leave Migrate alone and change the key name used by #2080823: Create API to discover config entities' soft dependencies and use this to present a confirm form on module uninstall into:

requires

As known from Composer and also extension system related issues.

benjy’s picture

benjy commented
Status: Needs work » Needs review

1: 2221767.1.patch queued for re-testing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

I'm not a huge fan of #6 whilst we have no commitment to change .info.yml files too.

benjy’s picture

benjy commented
Status: Needs review » Reviewed & tested by the community

I can't see any problem with renaming this if it helps the other issue.

We do have a test for the dependency system but it's in migrate_drupal MigrateDependenciesTest, probably because it tests by loading a few dependant migrations. It would be nice to add some more tests for the dependency system within the migrate api by extending our unit tests but that is obviously out of scope here.

chx’s picture

chx commented

This is OK with me. In an independent issue

+ // Migration dependencies defined in the migration storage controller can be

Um. I know that was the original doxygen ; we need a minor docs issue to change this to "Migration dependencies defined in the migration entities can be". I must have been one functioning braincell short when I wrote this.

  • Commit 743957c on 8.x by catch: 
    Issue #2221767 by alexpott: Migration configuration entities should not...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

#1 works for me. We can rename again later if we want to, but let's get the conflict out of the way.

Status: Fixed » Closed (fixed)

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