Problem/Motivation

When migrating taxonomy terms from Drupal 7 to 8, the field instance configuration for fields added to the taxonomy_term entity and the data within them is not coming across. I believe #2675470: D7 field instance settings for any fields not on nodes are not migrating covers the migration of field instance configuration for both taxonomy terms, users and other fieldable entities. This issue just covers the migration of the data within those fields for taxonomy terms.

The reason field data is not coming across is that the term migration plugin is extending DrupalSqlBase rather than FieldableEntity.

Proposed resolution

* Separate the existing term migration plugin into separate plugins for D6 and D7.
* Extend the D7 plugin from FieldableEntity
* Modify the prepareRow() method to pull in the fields - linking to the taxonomy_vocabulary table is necessary to get the entity bundle (or taxonomy_vocabulary machine_name) for this to work
* Due to the stub creation in the case of child terms being created before their parents, we need to ensure that 'iterator' plugin can handle this situation where $value is not an array or is NULL.

Remaining tasks

Probably check that other fieldable entities don't have similiar issues.

Create a change record (because taxonomy_term has been split into d6_taxonomy_term & d7_taxonomy_term).

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#79 interdiff-66-78.txt582 bytesmikeryan
#78 cck_field_data_not-2746671-78-8.2.x.patch37.74 KBjofitz
#78 interdiff_76-78.txt745 bytesjofitz
#76 cck_field_data_not-2746671-76-8.2.x.patch37.84 KBjofitz
#76 interdiff_66-76.txt737 bytesjofitz
#66 cck_field_data_not-2746671-66.patch37.73 KBjofitz
#66 interdiff_64-66.txt478 bytesjofitz
#64 cck_field_data_not-2746671-64.patch37.73 KBjofitz
#61 cck_field_data_not-2746671-61.patch38 KBjofitz
#61 interdiff_59-61.txt2.79 KBjofitz
#59 cck_field_data_not-2746671-59.patch40.02 KBjofitz
#59 interdiff_55-59.txt3.46 KBjofitz
#55 cck_field_data_not-2746671-55.patch39.83 KBjofitz
#51 cck_field_data_not-2746671-51.patch33.8 KBjofitz
#49 cck_field_data_not-2746671-49.patch30.01 KBjofitz
#49 interdiff.txt2.84 KBjofitz
#45 cck_field_data_not-2746671-45.patch30 KBjofitz
#45 interdiff.txt768 bytesjofitz
#44 cck_field_data_not-2746671-44.patch30.45 KBjofitz
#44 interdiff.txt13.84 KBjofitz
#41 2746671-taxonomy-migration-data-check.png150.01 KBchriscalip
#41 2746671-taxonomy-migration-success.png124.71 KBchriscalip
#38 cck_field_data_not-2746671-38.patch32.57 KBjofitz
#38 interdiff.txt1.4 KBjofitz
#36 cck_field_data_not-2746671-36.patch30.3 KBjofitz
#31 cck_field_data_not-2746671-31.patch27.99 KBjofitz
#27 cck_field_data_not-2746671-27.patch29.25 KBjofitz
#23 cck_field_data_not-2746671-23.patch24.43 KBtom friedhof
#21 cck_field_data_not-2746671-21.patch29.88 KBandrewmacpherson
#20 cck_field_data_not-2746671-20.patch29.76 KBhussainweb
#20 interdiff-19-20.txt5.4 KBhussainweb
#19 cck_field_data_not-2746671-19.patch37.77 KBhussainweb
#18 taxonomy-terms-migrate-d7-field-data-2746671-18-D8.patch37.73 KBjofitz
#18 interdiff.txt10.72 KBjofitz
#16 taxonomy-terms-migrate-d7-field-data-2746671-16-D8.patch10.72 KBjofitz
#16 interdiff.txt10.72 KBjofitz
#15 interdiff-2746671-11-15.txt5.28 KBandrewmacpherson
#15 taxonomy-terms-migrate-d7-field-data-2746671-15-D8.patch38.5 KBandrewmacpherson
#11 taxonomy-terms-migrate-d7-field-data-2746671-11-D8.patch38.38 KBjofitz
#11 interdiff.txt5.08 KBjofitz
#9 interdiff.txt11.46 KBjofitz
#9 taxonomy-terms-migrate-d7-field-data-2746671-9-D8.patch33.29 KBjofitz
#8 interdiff.txt12.18 KBjofitz
#8 taxonomy-terms-migrate-d7-field-data-2746671-8-D8.patch22.25 KBjofitz
#6 taxonomy-terms-migrate-d7-field-data-2746671-6-D8.patch10.06 KBstella
#4 taxonomy-terms-migrate-d7-field-data-2746671-4-D8.patch9.14 KBstella
#2 taxonomy-terms-migrate-d7-field-data-2746671-1-D8.patch0 bytesstella
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

stella created an issue. See original summary.

stella’s picture

Status: Needs review » Needs work

The last submitted patch, 2: taxonomy-terms-migrate-d7-field-data-2746671-1-D8.patch, failed testing.

stella’s picture

Status: Needs review » Needs work

The last submitted patch, 4: taxonomy-terms-migrate-d7-field-data-2746671-4-D8.patch, failed testing.

stella’s picture

Updated the migration templates

Status: Needs review » Needs work

The last submitted patch, 6: taxonomy-terms-migrate-d7-field-data-2746671-6-D8.patch, failed testing.

jofitz’s picture

Add tests for fieldable taxonomy terms. Separate d6 and d7 tests.

jofitz’s picture

* Enable processing of taxonomy term fields.
* Test migration of taxonomy term fields.

Status: Needs review » Needs work

The last submitted patch, 9: taxonomy-terms-migrate-d7-field-data-2746671-9-D8.patch, failed testing.

jofitz’s picture

Status: Needs work » Needs review
FileSize
5.08 KB
38.38 KB

Correct errors introduced into other tests.

jofitz’s picture

Issue tags: +migrate-d7-d8
vasi’s picture

Status: Needs review » Needs work

This is great work!

  1. Do we want a change record for this? Anybody who uses source 'taxonomy_term' is going go see a change.
  2. The d7_taxonomy_term migration will need a dependency on d7_field_instance.
  3. +++ b/core/modules/migrate/src/Plugin/migrate/process/Iterator.php
    @@ -23,14 +23,16 @@ class Iterator extends ProcessPluginBase {
    +    if (is_array($value)) {
    

    Let's not just silently return an empty array if the input is something totally different, eg: an integer. Can we just check for NULL?

  4. +++ b/core/modules/taxonomy/src/Plugin/migrate/Term.php
    @@ -0,0 +1,131 @@
    +            if (!isset($this->cckPluginCache[$field_type])) {
    +              $this->cckPluginCache[$field_type] = $this->cckPluginManager->createInstance($field_type, [], $this);
    +            }
    +            $info = $row->getSource();
    +            $this->cckPluginCache[$field_type]
    +              ->processCckFieldValues($this, $field_name, $info);
    

    This isn't tested anywhere.

  5. +++ b/core/modules/taxonomy/src/Plugin/migrate/Term.php
    @@ -0,0 +1,131 @@
    +        foreach ($profile_migration->getSourcePlugin() as $row) {
    +          $name = $row->getSourceProperty('name');
    +          $this->process[$name] = $name;
    +        }
    

    This is also not tested anywhere. Maybe we should do profile fields in a follow up.

  6. Is it ok that we have one migration, that does the field processing from all taxonomy vocabularies? Isn't it likely that some of the processes would fail on NULL input? I think we probably need to derive a migration for each bundle, like we do for nodes.

    This might have the benefit of letting us share code with D7NodeDeriver.

vasi’s picture

Please build on top of Drupal\migrate_drupal\Plugin\migrate\CckMigration, instead of building a custom process.

andrewmacpherson’s picture

This needed a re-roll following #2691641: D7 taxonomy description format not migrating being committed.

jofitz’s picture

Responses to @vasi's code review:

  1. I'm not sure how to create a change record - can someone else look into this, please?
  2. Added d7_taxonomy_term migration dependency on d7_field_instance.
  3. Replaced is_array() with !is_null()
  4. Add test for migration of cck field (term reference) on taxonomy term
  5. Created a follow-up ticket for profile_field migration (#2754457: Test migration of Taxonomy Term profile_fields)
  6. Created a follow-up ticket for deriving a migration for each bundle (#2754471: Write a deriver for migration of Taxonomy Term bundles)

Also now based on CckMigration.

Status: Needs review » Needs work

The last submitted patch, 16: taxonomy-terms-migrate-d7-field-data-2746671-16-D8.patch, failed testing.

jofitz’s picture

Status: Needs work » Needs review
FileSize
10.72 KB
37.73 KB

Uploaded correct patch.

hussainweb’s picture

First doing a reroll.

hussainweb’s picture

Here are a few tweaks. Mostly, they are changes in array syntax but there is also a missing field declaration. I also made the patch smaller (and easier to review) by detecting renames.

andrewmacpherson’s picture

Needs a re-roll since #2737607: Migration Source Plugins are not extendable because of ambiguous database field names.

Note: the patch in #20 applies cleanly against tag 8.1.7, for anyone who is already using this.

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

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

tom friedhof’s picture

Here is a rerolled patch that will apply cleanly against 8.1.8 or 8.1.x branch

Status: Needs review » Needs work

The last submitted patch, 23: cck_field_data_not-2746671-23.patch, failed testing.

The last submitted patch, 21: cck_field_data_not-2746671-21.patch, failed testing.

The last submitted patch, 21: cck_field_data_not-2746671-21.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 27: cck_field_data_not-2746671-27.patch, failed testing.

The last submitted patch, 27: cck_field_data_not-2746671-27.patch, failed testing.

The last submitted patch, 27: cck_field_data_not-2746671-27.patch, failed testing.

jofitz’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 31: cck_field_data_not-2746671-31.patch, failed testing.

The last submitted patch, 31: cck_field_data_not-2746671-31.patch, failed testing.

The last submitted patch, 31: cck_field_data_not-2746671-31.patch, failed testing.

jofitz’s picture

Re-roll (due to issues with git diff).

Status: Needs review » Needs work

The last submitted patch, 36: cck_field_data_not-2746671-36.patch, failed testing.

jofitz’s picture

Status: Needs review » Needs work

The last submitted patch, 38: cck_field_data_not-2746671-38.patch, failed testing.

jofitz’s picture

Status: Needs work » Needs review

Now passing all tests, appears to have been a fault in the testbot.

chriscalip’s picture

A shallow review test has pass : 1.
Scenario
D7 Install with 5 vocabularies.
Vocabulary asset_category has field field_cap_category_level
Vocabulary asset_status has field field_status_text
Migrating to clean D8 3.x Install.
Vocabulary asset_category has field field_category_level
Vocabulary asset_status has field field_status_text

Using migration map migrate_plus.migration.taxonomy_vocabulary.yml

langcode: en
status: true
dependencies: {  }
id: taxonomy_term
migration_group: carlyle
label: 'Taxonomy terms'
source:
  plugin: d7_taxonomy_term
process:
  tid: tid
  vid:
    plugin: migration
    migration: taxonomy_vocabulary
    source: vid
  name: name
  'description/value': description
  'description/format': format
  weight: weight
  parent_id:
    -
      plugin: skip_on_empty
      method: process
      source: parent
    -
      plugin: migration
      migration: taxonomy_term
  parent:
    plugin: default_value
    default_value: 0
    source: '@parent_id'
  changed: timestamp
  field_category_level: field_cap_category_level
  field_status_text: field_status_text
destination:
  plugin: 'entity:taxonomy_term'
migration_dependencies:
  required:
    - taxonomy_vocabulary
  optional: {  }

Screenshots :

a.) 2746671-taxonomy-migration-success.png , shows migration success for taxonomy_term and has no errors.
b.) 2746671-taxonomy-migration-data-check.png , shows field data values migrated also.

chriscalip’s picture

Another successful use of this patch : +1
Scenario

D7 Install with 5 vocabularies.
Vocabulary asset_category has field field_cap_category_level (list)(unlimited)
Vocabulary asset_status has field field_status_icon (imagefield)
Vocabulary conversation_category has field field_tester (entityreference)

Migrating to clean D8 8.2.0-rc1 Install.
Vocabulary asset_category has field field_category_level (list)(unlimited)
Vocabulary asset_status has field field_status_icon (imagefield)
Vocabulary conversation_category has field field_tester (entityreference)

langcode: en
status: true
dependencies: {  }
id: taxonomy_term
migration_group: carlyle
label: 'Taxonomy terms'
source:
  plugin: d7_taxonomy_term
process:
  tid: tid
  vid:
    plugin: migration
    migration: taxonomy_vocabulary
    source: vid
  name: name
  'description/value': description
  'description/format': format
  weight: weight
  parent_id:
    -
      plugin: skip_on_empty
      method: process
      source: parent
    -
      plugin: migration
      migration: taxonomy_term
  parent:
    plugin: default_value
    default_value: 0
    source: '@parent_id'
  changed: timestamp
  # select_list
  field_category_level: field_cap_category_level
  # image
  field_status_icon:
    plugin: iterator
    source: field_status_icon
    process:
      target_id: fid
      alt: alt
      title: title
      width: width
      height: height
  # entityreference node_page
  field_tester: field_tester
destination:
  plugin: 'entity:taxonomy_term'
migration_dependencies:
  required:
    - taxonomy_vocabulary
  optional:
    - node_page
    - file
mikeryan’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/migrate/src/Plugin/migrate/process/Iterator.php
    @@ -49,7 +51,7 @@ public function transform($value, MigrateExecutableInterface $migrate_executable
    -    $process = array('key' => $this->configuration['key']);
    +    $process = ['key' => $this->configuration['key']];
    

    Coding standard changes are out of scope here (and best addressed with core-wide scripting, as in #2776975: March 3, 2017: Convert core to array syntax coding standards for Drupal 8.3.x RC phase).

  2. +++ b/core/modules/taxonomy/migration_templates/d7_taxonomy_term.yml
    @@ -2,8 +2,9 @@ id: d7_taxonomy_term
    +class: Drupal\taxonomy\Plugin\migrate\Term
    
    +++ b/core/modules/taxonomy/src/Plugin/migrate/Term.php
    @@ -0,0 +1,61 @@
    +class Term extends CckMigration {
    

    I'm having trouble following this class, or understanding why we need a custom migration plugin class here. What's special about taxonomy terms as fieldable entities, compared to nodes, that requires this class? Is this trying to accomplish what the deriver classes do for nodes? I see a followup for a deriver, but why not use a deriver in the first place instead of a custom migration class?

    At the very least, some comments would be helpful.

  3. +++ b/core/modules/taxonomy/src/Plugin/migrate/Term.php
    @@ -0,0 +1,61 @@
    +        $definition['source']['plugin'] = 'profile_field';
    +        $profile_migration = $this->migrationPluginManager->createStubMigration($definition);
    

    I don't see what profiles have to do with taxonomy terms? D7 profile module didn't have any kind of taxonomy reference field as far as I can tell.

  4. +++ b/core/modules/taxonomy/src/Plugin/migrate/source/d6/Term.php
    @@ -0,0 +1,91 @@
    +class Term extends DrupalSqlBase {
    +
    +  /**
    +   * Name of the term data table.
    +   *
    +   * @var string
    +   */
    +  protected $termDataTable;
    +
    +  /**
    +   * Name of the term hierarchy table.
    +   *
    +   * @var string
    +   */
    +  protected $termHierarchyTable;
    

    If these are going to be entirely separate plugins (each inheriting from DrupalSqlBase), there's no reason to not hard-code the table names instead of having member variables for them. Of course, you could have a common base class sharing them.

  5. +++ b/core/modules/taxonomy/src/Plugin/migrate/source/d7/Term.php
    @@ -0,0 +1,109 @@
    +class Term extends FieldableEntity {
    

    Ah, right, here's why they can't have the common base class. They could share a trait though, but if not then again, just hard-code the table names directly.

jofitz’s picture

1. Removed change.
2. & 3. Replaced migration plugin with deriver.
4. & 5. Hard-coded table names.

jofitz’s picture

Sorry, I actually managed to miss @mikeryan's point 1 in the previous commit.

chriscalip’s picture

Shallow test. last patch unusable.

✝ vagrant@multi-vlad ✝ /var/www/site/docroot (master)
$ drush ms
SQLSTATE[HY093]: Invalid parameter number: number of bound variables does not match number of tokens: SELECT COUNT(*) AS expression
FROM
(SELECT DISTINCT td.*, tv.machine_name AS machine_name, 1 AS expression
FROM
{taxonomy_term_data} td
LEFT OUTER JOIN {taxonomy_vocabulary} tv ON td.vid = tv.vid
WHERE  (tv.machine_name = :db_condition_placeholder_0:db_condition_placeholder_1:db_condition_placeholder_2:db_condition_placeholder_3:db_condition_placeholder_4) ) subquery; Array
(
    [:db_condition_placeholder_0] => 1
    [:db_condition_placeholder_1] => 2
    [:db_condition_placeholder_2] => 3
    [:db_condition_placeholder_3] => 4
    [:db_condition_placeholder_4] => 5
)

 Group: D7 imports (capacitype)           Status  Total  Imported  Unprocessed  Last imported
 node_article                             Idle    28     0         28
 user_role                                Idle    7      0         7
 file                                     Idle    217    0         217
 user                                     Idle    941    0         941
 taxonomy_term                            Idle    N/A    0         N/A
 node_question                            Idle    34     0         34
 comment_answernode2questioncomment       Idle    73     0         73
 comment_answercomments2questioncomments  Idle    2      0         2
 node_contact                             Idle    140    0         140
 node_person2asset                        Idle    154    0         154
 node_locale2asset                        Idle    800    0         800

✝ vagrant@multi-vlad ✝ /var/www/site/docroot (master)
$ drush core-status
 Drupal version                  :  8.2.1
 Site URI                        :  http://default
 Database driver                 :  mysql
 Database hostname               :  localhost
 Database port                   :  3306
 Database username               :  vlad
 Database name                   :  c5db
 Drupal bootstrap                :  Successful
 Drupal user                     :
 Default theme                   :  stark
 Administration theme            :  seven
 PHP configuration               :  /etc/php5/cli/php.ini
 PHP OS                          :  Linux
 Drush script                    :  /usr/local/share/drush/drush
 Drush version                   :  8.1.0
 Drush temp directory            :  /tmp
 Drush configuration             :  /home/vagrant/.drush/drushrc.php
 Drush alias files               :
 Install profile                 :  minimal
 Drupal root                     :  /var/www/site/docroot
 Drupal Settings File            :  sites/default/settings.php
 Site path                       :  sites/default
 File directory path             :  sites/default/files
 Private file directory path     :  sites/default/files/private
 Temporary file directory path   :  /tmp
 Sync config path                :  sites/default/config
chriscalip’s picture

Looks like last patch has input options now are vocabulary.machine_name based from vocabulary.vid. Updating my test instance to test again last patch input requirements.

Still no joy.

✝ vagrant@multi-vlad ✝ /var/www/site/docroot (master)
$ drush ms
SQLSTATE[HY093]: Invalid parameter number: number of bound variables does not match number of tokens: SELECT COUNT(*) AS expression
FROM 
(SELECT DISTINCT td.*, tv.machine_name AS machine_name, 1 AS expression
FROM 
{taxonomy_term_data} td
LEFT OUTER JOIN {taxonomy_vocabulary} tv ON td.vid = tv.vid
WHERE  (tv.machine_name = :db_condition_placeholder_0:db_condition_placeholder_1:db_condition_placeholder_2:db_condition_placeholder_3) ) subquery; Array
(
    [:db_condition_placeholder_0] => asset_category
    [:db_condition_placeholder_1] => asset_status
    [:db_condition_placeholder_2] => tyr_map_categories
    [:db_condition_placeholder_3] => conversation_category
)
chriscalip’s picture

Status: Needs review » Needs work
jofitz’s picture

Correct erroneous assumption (introduced in #44): Handle Term migration configuration Vocabulary as an array.

@chriscalip, would you mind retesting, please.

Status: Needs review » Needs work

The last submitted patch, 49: cck_field_data_not-2746671-49.patch, failed testing.

jofitz’s picture

Re-roll.

mikeryan’s picture

Assigned: Unassigned » mikeryan
mikeryan’s picture

Assigned: mikeryan » Unassigned
Status: Needs review » Needs work
Issue tags: +Needs reroll

That target just keeps moving...

+++ b/core/modules/taxonomy/src/Plugin/migrate/D7TaxonomyTermDeriver.php
@@ -0,0 +1,130 @@
+   * D7NodeDeriver constructor.

copypasta

jofitz’s picture

Assigned: Unassigned » jofitz

Major re-roll in progress!

jofitz’s picture

Assigned: jofitz » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
39.83 KB

Re-roll.

mikeryan’s picture

Status: Needs review » Reviewed & tested by the community

Looks good, and I've successfully tested locally from a D7 environment where I added a couple of fields to the tags vocabulary. Good to go, thanks!

catch’s picture

+++ b/core/modules/taxonomy/src/Plugin/migrate/D7TaxonomyTermDeriver.php
@@ -0,0 +1,130 @@
diff --git a/core/modules/taxonomy/src/Plugin/migrate/source/Term.php b/core/modules/taxonomy/src/Plugin/migrate/source/Term.php

diff --git a/core/modules/taxonomy/src/Plugin/migrate/source/Term.php b/core/modules/taxonomy/src/Plugin/migrate/source/Term.php
deleted file mode 100644

deleted file mode 100644
index 0b2a5c3..0000000

index 0b2a5c3..0000000
--- a/core/modules/taxonomy/src/Plugin/migrate/source/Term.php

--- a/core/modules/taxonomy/src/Plugin/migrate/source/Term.php
+++ /dev/null

+++ /dev/null
+++ /dev/null
@@ -1,100 +0,0 @@

@@ -1,100 +0,0 @@
-<?php
-
-namespace Drupal\taxonomy\Plugin\migrate\source;

Should this be kept for bc until 8.4.x?

mikeryan’s picture

Status: Reviewed & tested by the community » Needs work

Should this be kept for bc until 8.4.x?

Yes, let's keep this so we don't break BC. Unlike other typical cases where the BC layer can wrap the new stuff, since the one plugin is splitting in two, I think we need to keep it as-is.

+++ b/core/modules/taxonomy/src/Plugin/migrate/D7TaxonomyTermDeriver.php
@@ -0,0 +1,130 @@
+        $values['source']['vocabulary'] = $bundle;

+++ b/core/modules/taxonomy/src/Plugin/migrate/source/d6/Term.php
@@ -0,0 +1,74 @@
+    if (isset($this->configuration['vocabulary'])) {
+      $query->condition('td.vid', $this->configuration['vocabulary'], 'IN');
+    }

+++ b/core/modules/taxonomy/src/Plugin/migrate/source/d7/Term.php
@@ -0,0 +1,84 @@
+    if (isset($this->configuration['vocabulary'])) {
+      $query->condition('tv.machine_name', $this->configuration['vocabulary'], 'IN');
+    }

+++ b/core/modules/taxonomy/tests/src/Kernel/Plugin/migrate/source/d6/TermSourceWithVocabularyFilterTest.php
@@ -0,0 +1,56 @@
+    $tests[0]['configuration'] = [
+      'vocabulary' => [5],
+    ];

+++ b/core/modules/taxonomy/tests/src/Kernel/Plugin/migrate/source/d7/TermSourceWithVocabularyFilterTest.php
@@ -0,0 +1,56 @@
+    $tests[0]['configuration'] = [
+      'vocabulary' => ['tags'],
+    ];

Per https://www.drupal.org/node/2785201#comment-11754887, could we change the new source option key from 'vocabulary' to 'bundle'? It'll be beneficial in the long run, I think, to have a standard option name here for any entity type which may be filtered by bundle.

Thanks.

jofitz’s picture

Status: Needs work » Needs review
FileSize
3.46 KB
40.02 KB

Changed 'vocabulary' to 'bundle', as recommended.

mikeryan’s picture

Status: Needs review » Needs work

Changed 'vocabulary' to 'bundle', as recommended.

Thanks.

Should this be kept for bc until 8.4.x?

Yes, let's keep this so we don't break BC. Unlike other typical cases where the BC layer can wrap the new stuff, since the one plugin is splitting in two, I think we need to keep it as-is.

Let's restore (and deprecate) the old version-agnostic plugin.

jofitz’s picture

Status: Needs work » Needs review
FileSize
2.79 KB
38 KB

Restored (and deprecated) the old version-agnostic plugin.

mikeryan’s picture

Status: Needs review » Reviewed & tested by the community

OK, think this is good to go!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 61: cck_field_data_not-2746671-61.patch, failed testing.

jofitz’s picture

Status: Needs work » Needs review
FileSize
37.73 KB

Re-roll.

(let's get this committed super-quickly so i don't have to re-roll yet again :p )

Status: Needs review » Needs work

The last submitted patch, 64: cck_field_data_not-2746671-64.patch, failed testing.

jofitz’s picture

Status: Needs review » Reviewed & tested by the community

I think this can safely be returned to RTBC - it was only a minor re-roll.

  • catch committed 11cb2b7 on 8.3.x
    Issue #2746671 by Jo Fitzgerald, stella, andrewmacpherson, hussainweb,...

  • catch committed d002382 on 8.2.x
    Issue #2746671 by Jo Fitzgerald, stella, andrewmacpherson, hussainweb,...
catch’s picture

Version: 8.3.x-dev » 8.2.x-dev
Status: Reviewed & tested by the community » Fixed

Fixed a redundant use statement on commit.
Committed/pushed to 8.3.x and cherry-picked to 8.2.x. Thanks!

  • xjm committed eb957dc on 8.2.x
    Revert "Issue #2746671 by Jo Fitzgerald, stella, andrewmacpherson,...

  • xjm committed 1d4ffea on 8.3.x
    Revert "Issue #2746671 by Jo Fitzgerald, stella, andrewmacpherson,...
xjm’s picture

Status: Fixed » Needs work

This caused HEAD failures on 8.2.x that I was able to reproduce locally. alexpott was not able to reproduce the fail on 8.3.x. I've reverted both commits; we may need to do something different to backport or fix something else in the patch.

xjm’s picture

Oh and here is the failing 8.2.x result: https://www.drupal.org/pift-ci-job/536475

jofitz’s picture

8.2.x fails because it cannot handle scalers with an IN condition (while 8.3.x has additional code to handle this).

What is the best way to resolve this? A separate patch for 8.2.x? Or include a line that is redundant in 8.3.x so that it works for both?

jofitz’s picture

Status: Needs work » Needs review
FileSize
737 bytes
37.84 KB

Here is the slightly adjusted version that should work for 8.2.x, but has a redundant line for 8.3.x.

I'll let someone else decide how they want to handle these!

mikeryan’s picture

Ah, I was working on debugging this today, you got there first:P.

Slightly simpler would be casting with (array).

mikeryan’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
582 bytes

Adding interdiff against the originally-committed patch.

I think we're good to get this back in (pending results of the full battery of tests), thanks Jo!

catch’s picture

I've sent a couple of environments for re-test.

  • catch committed 9835034 on 8.3.x
    Issue #2746671 by Jo Fitzgerald, stella, andrewmacpherson, hussainweb,...

  • catch committed 36fe2d4 on 8.2.x
    Issue #2746671 by Jo Fitzgerald, stella, andrewmacpherson, hussainweb,...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.3.x and cherry-picked to 8.2.x. Thanks!

Status: Fixed » Closed (fixed)

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

jhood’s picture

I created another issue at https://www.drupal.org/node/2873742 to add the @see link to the change record for this issue, but was unable to locate one.

mikeryan’s picture

Status: Closed (fixed) » Needs work
Issue tags: +Needs change record

The change record needs to be added on this issue.

heddn’s picture

Added a change record.

mikeryan’s picture

Status: Needs work » Fixed
Issue tags: -Needs change record

CR published.

Status: Fixed » Closed (fixed)

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