Problem/Motivation

D6 taxonomy vocabularies have two settings (tags, and multiple) that are related to field cardinality. The descriptions for the fields are the following.

Tags: Terms are created by users when submitting posts by typing a comma separated list.
Multiple: Allows posts to have more than one term from this vocabulary (always true for tags).

(Source: http://cgit.drupalcode.org/drupal/tree/modules/taxonomy/taxonomy.admin.i...)

Proposed resolution

Set field cardinality to Unlimited if tags or multiple is enabled, otherwise 1.

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#18 interdiff-2616220-15-18.txt776 bytesquietone
#18 2616220-18.patch4.9 KBquietone
#15 interdiff-2616220-7-15.txt1.6 KBquietone
#15 2616220-15.patch4.63 KBquietone
#7 2616220-7.patch4.36 KBwebflo
#4 2616220-4.patch3.98 KBwebflo
#3 2616220-2.patch1.44 KBwebflo
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

webflo created an issue. See original summary.

webflo’s picture

webflo CreditAttribution: webflo at UEBERBIT GmbH commented
Status: Active » Needs review
Issue tags: +rc eligible
webflo’s picture

webflo CreditAttribution: webflo at UEBERBIT GmbH commented
FileSize
2616220-2.patch1.44 KB
webflo’s picture

webflo CreditAttribution: webflo at UEBERBIT GmbH commented
FileSize
2616220-4.patch3.98 KB

The last submitted patch, 3: 2616220-2.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 4: 2616220-4.patch, failed testing.

webflo’s picture

webflo CreditAttribution: webflo at UEBERBIT GmbH commented
Status: Needs work » Needs review
FileSize
2616220-7.patch4.36 KB

I had to change the db dump because there are node revisions with multiple terms.

Status: Needs review » Needs work

The last submitted patch, 7: 2616220-7.patch, failed testing.

The last submitted patch, 7: 2616220-7.patch, failed testing.

webflo’s picture

webflo CreditAttribution: webflo at UEBERBIT GmbH commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 7: 2616220-7.patch, failed testing.

webflo’s picture

webflo CreditAttribution: webflo at UEBERBIT GmbH commented
Status: Needs work » Needs review
benjy’s picture

benjy CreditAttribution: benjy commented

Thanks, patch and tests look good, RTBC for me, just one small thing:

+++ b/core/modules/taxonomy/src/Plugin/migrate/source/d6/Vocabulary.php
@@ -72,6 +72,7 @@ public function prepareRow(Row $row) {
+    $row->setSourceProperty('cardinality', ($row->getSourceProperty('tags') == 1 || $row->getSourceProperty('multiple') == 1) ? -1 : 1);

Shall we add a constant for "1" here?

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Issue tags: +migrate-d6-d8
quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
FileSize
2616220-15.patch4.63 KB
interdiff-2616220-7-15.txt1.6 KB

What is the advantage of adding a constant for 1?

I think this should use the constant for unlimited cardinality.

benjy’s picture

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

Sorry, -1 makes much more sense. RTBC

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/modules/taxonomy/tests/src/Unit/Migrate/d6/VocabularyTest.php
@@ -39,6 +39,7 @@ class VocabularyTest extends MigrateSqlSourceTestCase {
+      'cardinality' => -1,

This should also use the constant.

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Status: Needs work » Needs review
FileSize
2616220-18.patch4.9 KB
interdiff-2616220-15-18.txt776 bytes

Yes, it should.

benjy’s picture

benjy CreditAttribution: benjy commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -rc eligible

Great

  • catch committed e660168 on 8.2.x 
    Issue #2616220 by webflo, quietone: Obey field cardinality in vocabulary...

  • catch committed 5bf8ff7 on 8.0.x 
    Issue #2616220 by webflo, quietone: Obey field cardinality in vocabulary...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to all three 8.x branches, thanks!

Adding review credit post-commit.

Status: Fixed » Closed (fixed)

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