Problem/Motivation

As seen in #2647470: Write tests, The migrate UI is currently writing invalid configuration. In order for the module's functionality to become part of core we need to fix that.

Proposed Resolution

Add database_state_key to the generic migrate_source schema - it is used in Drupal\migrate\Plugin\migrate\source\SqlBase
Add source_base_path to the schema for file destination - it is used in Drupal\file\Plugin\migrate\destination\EntityFile.

Note that #2577871: d7_file plugin should receive source_base_path configuration option proposes removing this - which I think is correct - but we can't put a test for the UI without putting this in. Therefore let's add the schema and remove it in that issue if we ever get round to it.

Remaining Tasks

API Changes

None

Data Model Changes

None - this configuration exists it just was never saved by core.

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

alexpott created an issue. See original summary.

alexpott’s picture

Status: Active » Needs review
FileSize
591 bytes
Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Normally I would advocate for test coverage for this, but we are about to add that in #2647470: Write tests. So let's get this in.

alexpott’s picture

Title: source_base_path missing from file destination schema » A missing schema to support migrate UI - source_base_path and database_state_key
Status: Reviewed & tested by the community » Needs work

@Gábor Hojtsy I just worked out that the 'database_state_key' is also used by core migrations. Let's add all the missing stuff in one go.

alexpott’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
577 bytes
1.14 KB
alexpott’s picture

Related issues: +#2647470: Write tests
benjy’s picture

+++ b/core/modules/migrate/config/schema/migrate.data_types.schema.yml
@@ -25,6 +25,9 @@ migrate_source:
+    database_state_key:

So now all migrate sources will have this schema applied, even ones not in core right? Is that correct.

alexpott’s picture

@benjy yes - is that a problem? It just says that the key might be there.

alexpott’s picture

I discussed this with @benjy on IRC, tldr; benjy's valid concerns about this affecting exported configuration are unfounded.

11:18 alexpott
benjy: is there anything I have to do to address your feedback on https://www.drupal.org/node/2678564?
11:35 benjy
alexpott: i was wondering if it was something that could live in ui module?
11:35 alexpott
benjy: not really
11:35 benjy
Not sure how easy it is to add to existing types though
11:35 alexpott
benjy: it is possible just really not recommended
11:36 alexpott
benjy: and this covering stuff from Drupal\migrate\Plugin\migrate\source\SqlBase
11:36 alexpott
benjy: once migration become plugins this disappears
11:36 alexpott
benjy: after we find a way to inject these types of setting in a different way
11:37 benjy
alexpott: with this approach though, even non sql sources would have that key in an export though?
11:37 alexpott
benjy: nope
11:37 benjy
alexpott: yeah agreed. Plugins solve this
11:38 alexpott
benjy: schema define possible keys - not what keys
11:38 alexpott
benjy: so it does affect export
11:40 benjy
alexpott: ah ok, I thought if you didn't specify export keys, it fell back to using schema
11:40 benjy
alexpott: if it won't end up in the export, then that was my only question/concern
11:41 alexpott
benjy: only for top level config entity properties - which this is not
Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

database_state_key is indeed defined in SqlBase abstract source base class:

    if (!isset($this->database)) {
      // See if the database info is in state - if not, fallback to
      // configuration.
      if (isset($this->configuration['database_state_key'])) {
        $this->database = $this->setUpDatabase($this->state->get($this->configuration['database_state_key']));
      }

The rest of the source classes inherit from there. SqlBase inherits from SourcePluginBase, which does have a whole bunch of other configuration possibilities, eg. track_changes, cache_key, skip_count, etc. I don't see those in the schema either, assuming migrate_source maps to SourcePluginBase/SqlBase in some way. I assume there is no test coverage to exercise those being saved (yet). Given the time pressures with getting the UI in now, I think its fine to fix these keys only for now. It would be nice to have a more nuanced solution for the different source plugins and the missing base source plugin keys (in another issue).

source_base_path is more straightforward given it only appears with file migrations, so its effect is limited.

  • xjm committed 1703af3 on 8.1.x
    Issue #2678564 by alexpott, Gábor Hojtsy: A missing schema to support...
xjm’s picture

Status: Reviewed & tested by the community » Fixed

Committed 1703af3 and pushed to 8.1.x. Thanks!

I did not commit this issue to 8.0.x because it is a data model fix with no upgrade path and I don't think it's necessary to fix it in 8.0.x now anyway. If it needs to be backported, we can reopen for that.

Status: Fixed » Closed (fixed)

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