Note: This is a clone from https://www.drupal.org/node/2225781 that deals with taxonomy vocabularies. This issue deals with taxonomy terms.
Problem/Motivation
i18ntaxonomy sub module adds two fields to the {term_data} that take care about the translations, we need to create the term entities using the new entity API.
Proposed resolution
We need to create a custom source plugin that takes into account these new fields in {term_data} and also the i18nstrings module features so we can use the new column trid for the migration.
Original report by Ryan Weal
Like nodes, taxonomies can use entity translation. Notes above in nodes should apply.
Comment | File | Size | Author |
---|---|---|---|
#46 | 2784371-46.patch | 17.44 KB | jofitz |
#46 | interdiff-43-46.txt | 755 bytes | jofitz |
#43 | interdiff.txt | 656 bytes | quietone |
#43 | 2784371-43.patch | 17.57 KB | quietone |
#41 | interdiff.txt | 12.26 KB | quietone |
Comments
Comment #2
Gábor HojtsyComment #4
quietone CreditAttribution: quietone as a volunteer commentedLooks like adding translations for taxonomy terms needs to be added to the fixture.
Comment #5
Gábor HojtsyThis needs the same config code as #2225781: Migrate D6 i18n taxonomy vocabularies, so postponed on #2225717: Add config translation support to migrations and implement for Drupal 6 user profile fields the same way.
Comment #7
DamienMcKennaCross-referencing the issue that's blocking this one.
Comment #8
quietone CreditAttribution: quietone as a volunteer commented#2225717: Add config translation support to migrations and implement for Drupal 6 user profile fields is in, no loner postponed.
Comment #9
quietone CreditAttribution: quietone as a volunteer commentedShould be able to post a first patch in a few days.
Comment #10
quietone CreditAttribution: quietone as a volunteer commentedHere is a rough patch. All the files should be here, the source plugin, migration, tests and changes to the fixture. Although the changes to the fixture needs a closer look. The source plugin test passes locally but the not i18nTaxonomyTermTest. That results in the error below.
No reason to run the testbot.
1) Drupal\Tests\taxonomy\Kernel\Migrate\d6\MigrateI18nTaxonomyTermTest::testI18nTaxonomyTerms
SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate 6-0' for key 'PRIMARY': INSERT INTO {taxonomy_term_hierarchy} (tid, parent) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1), (:db_insert_placeholder_2, :db_insert_placeholder_3); Array
(
[:db_insert_placeholder_0] => 6
[:db_insert_placeholder_1] => 0
[:db_insert_placeholder_2] => 6
[:db_insert_placeholder_3] => 0
)
(/opt/sites/d8/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php:770)
Failed asserting that false is true.
Comment #11
jofitz CreditAttribution: jofitz at ComputerMinds commentedHere's a little tweak that might shed some light, @quietone (but it still fails, although differently now, hence still not running the testbot).
Comment #12
jofitz CreditAttribution: jofitz at ComputerMinds commentedGot it! @quietone you were so close.
#teamwork
Comment #14
jofitz CreditAttribution: jofitz at ComputerMinds commentedEdited a couple of tests to reflect changes to fixture.
I have also reverted a couple of fixture changes that were causing test failures:
@quietone let me know if these should be retained and we'll have edit the tests instead.
Comment #16
jofitz CreditAttribution: jofitz at ComputerMinds commentedEntity count change and a typo.
Comment #17
quietone CreditAttribution: quietone as a volunteer commentedA few more fixes.
1) The yml ids
2) Added a requirement of d6_taxonomy_term to d6_i18n_taxonomy_term
3) Added tests for the parents to the Migration test. The parents are gathered in prepareRow, and should not be changed from the d6_taxonomy_term migration. I guess it isn't strictly necessary to include the parents in the migration.
That still leaves a closer examination of the changes to the fixture.
Comment #18
quietone CreditAttribution: quietone as a volunteer commentedJust discovered that there is an i18ntaxonomy_vocabulary variable. Anyone know if this needs to be migrated and even better, to what?
On a quick look, I think it is storing these for each vocabulary.
/**
* Modes for multilingual vocabularies.
*/
// No multilingual options
define('I18N_TAXONOMY_NONE', 0);
// Localizable terms. Run through the localization system.
define('I18N_TAXONOMY_LOCALIZE', 1);
// Predefined language for a vocabulary and its terms.
define('I18N_TAXONOMY_LANGUAGE', 2);
// Per-language terms, translatable (referencing terms with different languages) but not localizable.
define('I18N_TAXONOMY_TRANSLATE', 3);
Comment #19
quietone CreditAttribution: quietone as a volunteer commentedTrim some more unnecessary changes to the fixture.
Comment #21
quietone CreditAttribution: quietone as a volunteer commentedAh, apparently I wasn't working from HEAD in #17, removing changes to the Block migration test.
Comment #22
jofitz CreditAttribution: jofitz at ComputerMinds commentedPatch no longer applies. Re-rolled.
Comment #23
mikeryanComment #24
jhodgdonJust a question/note... On #2793947-14: D6 migration testing with i18n, today I tested migrating a d6 site that used i18n taxonomy and had some translated nodes and translated taxonomy terms.
I noticed that the Spanish translated nodes that had translated taxonomy terms attached to them, the migrated nodes didn't get the taxonomy terms associated. The English ones came through OK.
So... I just want to ask whether this issue/patch will take care of that problem, or is it just about migrating the terms themselves? I don't see anything in the test in this patch that addresses taxonomy fields on nodes. Do we need a separate issue? If so, is there one already? I don't see one in Related... Thanks!
Comment #25
jhodgdonActually, given the breakdown on the parent meta issue #2208401: [META] Remaining multilingual migration paths, I am fairly certain it should be a new issue, at least for a test. So I filed:
#2859297: Migrate taxonomy term references for D6 Node translations
Comment #26
mikeryanFor nodes the translation migration corresponding to d6_node is d6_node_translation - I think for consistency we should name this d6_taxonomy_term_translation.
This source plugin is nearly identical to the d6_taxonomy_term plugin - it should inherit from that plugin class, then it only needs to override fields() to add 'language' and 'trid' to parent::fields().
Comment #27
jofitz CreditAttribution: jofitz at ComputerMinds commentedComment #28
jofitz CreditAttribution: jofitz at ComputerMinds commentedRenamed the migrate source plugin d6_i18n_taxonomy_term (and any related files and tests).
Comment #29
mikeryanLooks good to me, thanks!
Comment #32
quietone CreditAttribution: quietone as a volunteer commentedSadly, postponing this because the source plugin test is returning false positives. #2862006: MigrateSourceTestBase returns false positives for most plugin tests
Comment #33
heddnComment #34
jofitz CreditAttribution: jofitz at ComputerMinds commentedNo longer blocked.
Comment #36
Gábor HojtsyComment #37
quietone CreditAttribution: quietone as a volunteer commentedReroll
Comment #38
phenaproximaSelf-assigning for review.
Comment #39
phenaproximaGenerally looking pretty good!! I'm really happy to see this coming along, and I'm sorry it took me forever to review it.
This should mention the translations. So maybe "Taxonomy term translations" or "Taxonomy terms (translated)".
Nit: The should be a comma after the word "migration".
I don't understand why these are two separate properties; couldn't we simply add default_value to parent_id's pipeline? If not, we need a comment as to why.
For strict YAML compatibility, 'entity:taxonomy_term' should be single-quoted.
This class doesn't do much; I feel like we could merge all this directly into Term. If language is a column added by the i18n module, we could probably add a configuration key to Term which specifies that language and trid should be included, or check for the existence of the columns in the source table.
When will we be sure? :)
We should use assertSame().
Should be "Tests migration of translated taxonomy terms."
Needs @inheritdoc.
"id" should be "ID".
This should be assertInstanceOf().
Let's use assertSame() here.
I feel like the array_filter() call is a bit dangerous. If there are empty parent IDs, this could indicate some sort of problem with the hierarchy. Can we add a comment explaining why we're calling array_filter()?
Can we rename this testTranslatedTaxonomyTerms(), for readability?
If we merge the TermTranslation plugin into Term, as I suggested before, we can merge all of this into Term's test as another "pass" from the data provider.
Comment #40
quietone CreditAttribution: quietone as a volunteer commentedWorking on this. But it is taking awhile due to my cold.
Comment #41
quietone CreditAttribution: quietone as a volunteer commented1. Fixed
2. Let's make that change to all 15 occurrences of that sentence in a separate issue. Those lines were added by #2818157: Add comments to remove entity ids in migration.
3. This is just copy/paste from d6_taxonomy_term.yml. It looks like that was discussed herė https://www.drupal.org/node/2744639#comment-11285559. Í've only to find that discussion, not read it, as it is time to take me and cold to bed for a nap.
4. The existing migrations throughout core are not using single quotes. Maybe we should have an separate issue to meet strict YAML compatibility requirements.
5, 15. The class has been removed, added a 'translations' configuration key.
6. Oops, and fixed.
7. Fixed
8. Fixed
9. Fixed
10. Fixed
11. Fixed
12. Fixed
13. The hierarchy tests comes from d7/MigrateTaxonomyTermTest. It too was added in #274639: Unable to create fields / groups ... Possible table problem?.
14. Fixed
And I'm wondering if the destination should also use the key 'translations'.
Comment #43
quietone CreditAttribution: quietone as a volunteer commentedFix @covers to test the correct plugin.
Comment #44
phenaproximaOkay, I'm happy with the work done in #41. Proceed!
Comment #45
Gábor HojtsyDo we need all of these modules? Ones surprising me are: comment, datetime, image, locale, menu_ui, telephone.
Comment #46
jofitz CreditAttribution: jofitz at ComputerMinds commentedI did some digging and @Gábor Hojtsy is mostly correct: comment, datetime, image, locale and telephone are unnecessary. BUT a schema within menu_ui is required. In addition, content_translation, link and text are unnecessary.
Comment #47
phenaproximaNice catch by Gábor, good to see this a little cleaner :) Back to RTBC!
Comment #49
Gábor HojtsySuperb, thanks a lot!
Comment #50
jhodgdonI tested the latest 8.4.x today by migrating a d6 site that had translated taxonomy terms (using the d6 i18n taxonomy contrib module). The taxonomy terms did not come through with their translations. So, I don't think this fix actually works?
Comment #51
jhodgdonSee #2859297-6: Migrate taxonomy term references for D6 Node translations for details of how I tested this.
Comment #52
jhodgdonI just looked at the last patch on this issue. I don't see anything in that patch that deals with *translations* of terms. I see some code and test fixtures that deal with taxonomy term *language*, but nothing for my use case, which is having a particular taxonomy vocabulary whose terms have been translated from English into Spanish using the i18n_taxonomy module.
Was this issue supposed to cover that use case or do we need to create a new issue? Because as far as I can tell, the migration in this patch just covers migrating the language of a given term, not term translations.
Comment #53
Gábor HojtsyRight, so i18ntaxonomy alters the terms/vocabularies schemas a bit:
(from http://cgit.drupalcode.org/i18n/tree/i18ntaxonomy/i18ntaxonomy.install?h...)
This issue does in fact only cover a bit of that functionality, where the term language is migrated. It still did not cover trid (taxonomy term translation sets where the terms are different, although I don't think core can do that in Drupal 8). It also did not cover translations which is also an i18ntaxonomy option. So yeah those would need their own issues.
All the options listed at http://cgit.drupalcode.org/i18n/tree/i18ntaxonomy/i18ntaxonomy.module?h=... (for Drupal 6).
Let's not reopen this issue, which did cover one of the options, and open/repurpose new ones :)
Comment #54
Gábor HojtsySome key words in the title.
Comment #55
jhodgdonFair enough. #2886609: Migrate translations for D6 i18n taxonomy 'localized' terms is created. I'll add it to the parent issue too.
Comment #59
quietone CreditAttribution: quietone as a volunteer commentedThis issue does add migrations for term languages, just not the localized one. Updating the title.