Problem/Motivation

#2340401: Fill commented entity during stub comment saving shows that non-ID yet required fields break the migration process plugin.

Proposed resolution

  1. Move no_stub to the destination plugin. It's not a per-usage thing; there are things which simply can't be stubbed in a sensible way. Filter formats is an example. This is Win(TM) 1.
  2. Stub process needs to equal the complete process instead of just IDs. $process = $migration->get('process')
  3. If this breaks some processing then we need to provide sensible defaults. This is doable since the source configuration itself is the default even if it's mostly just used for constants. It can provide a default for any key. Note how we didn't need to resort to this.
  4. We need to add the defaults to the stub row $stub_row = new Row($values + $migration->get('source'), $source_ids);. The availability of constants for stub rows is Win(TM) 2. This is something the parent issue was unable to resolve cleanly.
  5. Finally the Entity destination should be changed to use only the ids achieving the same result as before patch. However, since the constants now are available inside the destination, comment can override the new method for this and use entity_type as well.

Remaining tasks

Make the patch pass :) Make exception tidier perhaps?

User interface changes

API changes

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

chx’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, new_m.patch, failed testing.

benjy’s picture

Status: Needs work » Needs review
FileSize
6.47 KB
1.23 KB

New patch fixes the fatal. The MigrationTest really isn't testing much to be honest. It was previously using the no_stub to skip most of the code in transform() and claiming to be a test for no_stub.

I vote for just deleting this test entirely? We have plenty of integration tests using the migrate process plugin.

Status: Needs review » Needs work

The last submitted patch, 3: 2411233-3.patch, failed testing.

benjy’s picture

Status: Needs work » Needs review
FileSize
7.05 KB
597 bytes

Added schema.

Status: Needs review » Needs work

The last submitted patch, 5: 2411233-5.patch, failed testing.

chx’s picture

Crappy tests can be deleted yes.

benjy’s picture

Status: Needs work » Needs review
FileSize
7.12 KB
792 bytes

The latest fails are because "plugin" is in the source but not in the values. That means we can remove it from the source or add it to the values. I went with the latter.

benjy’s picture

FileSize
7.81 KB
2 KB

Removed the test.

The last submitted patch, 8: 2411233-8.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 9: 2411233-9.patch, failed testing.

chx’s picture

Issue summary: View changes
FileSize
7.74 KB
864 bytes

Bah, you shouldn't listen to me :) here's the relevant code from Source

    $source_configuration = $this->migration->get('source');

    while ($this->getIterator()->valid()) {
      $row_data = $this->getIterator()->current() + $source_configuration;
      $this->getIterator()->next();
      $row = new Row($row_data, $this->migration->getSourcePlugin()->getIds(), $this->migration->get('destinationIds'));

therefore the source configuration needs to be added to the values not the ids :/

chx’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 12: 2411233_12.patch, failed testing.

chx’s picture

Invalid argument supplied for foreach()
Drupal\migrate\Plugin\migrate\process\Iterator->transform(NULL, Object, Object, 'filters')
Drupal\migrate\MigrateExecutable->processRow(Object, Array) 
Drupal\migrate\Plugin\migrate\process\Migration->transform(NULL, Object, Object, 'body/format') 

at first look, this means that filters needs a default array in the source (easy) but at second look, is this trying to stub a format?? Didn't we (try to) ban that?

andypost’s picture

@chx maybe add a parent migration to import filter and pass stubbing?

chx’s picture

d6_comment and d6_custom_block both depend on d6_filter_format already

benjy’s picture

Status: Needs work » Needs review
FileSize
10.6 KB
3.17 KB

Some fixes.

Status: Needs review » Needs work

The last submitted patch, 18: 2411233-18.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
12.53 KB
2.31 KB

See BlockPluginId: ->transform($delta where $delta is boxes.bid and not blocks.bid. The test confused blocks.bid for boxes.bid. Also, custom blocks can't be stubbed since there's a unique index containing info.

As for comments, as the issue summary says, the source for comments.created (timestamp) needs a default. I went with 0.

I am leaving file for others :)

Status: Needs review » Needs work

The last submitted patch, 20: 2411233_20.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
12.52 KB

Status: Needs review » Needs work

The last submitted patch, 22: 2411233_20.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
12.53 KB
569 bytes

Hrm, file is not failing? Let's try this; the latest fail is trivial to fix.

Status: Needs review » Needs work

The last submitted patch, 24: 2411233_24.patch, failed testing.

benjy’s picture

Status: Needs work » Needs review
FileSize
13.3 KB
2.48 KB

New patch attached.

benjy’s picture

Status: Needs review » Needs work

Posted the last patch a little early, new patch coming soon.

benjy’s picture

Status: Needs work » Needs review
FileSize
13.36 KB
1.03 KB

OK, try this.

The last submitted patch, 26: 2411233-26.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 28: 2411233-28.patch, failed testing.

benjy’s picture

Status: Needs work » Needs review
FileSize
14.18 KB
1.47 KB

OK, this should be closer.

benjy’s picture

FileSize
13.34 KB
859 bytes

Accidentally left some changes in the Comment entity.

The last submitted patch, 31: 2411233-30.patch, failed testing.

chx’s picture

Issue summary: View changes
hosef’s picture

Status: Needs review » Reviewed & tested by the community

This looks good. It works for me, and all tests pass.

chx’s picture

Issue summary: View changes
alexpott’s picture

Status: Reviewed & tested by the community » Fixed

migration changes are not blocked in beta. Committed 9eb9661 and pushed to 8.0.x. Thanks!

  • alexpott committed 9eb9661 on 8.0.x
    Issue #2411233 by benjy, chx: Stub in migration process plugin does not...

Status: Fixed » Closed (fixed)

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