Problem/Motivation

Taxonomy references was converted to ER filed but Vocabulary::postSave() still trying to update allowed_values setting that does not exists in ER field

So when vocabulary machine name is changed warning apears

Warning: Invalid argument supplied for foreach() in Drupal\taxonomy\Entity\Vocabulary->postSave() (line 144 of core/modules/taxonomy/src/Entity/Vocabulary.php).

Drupal\taxonomy\Entity\Vocabulary->postSave(Object, 1)

Proposed resolution

Use field config and ER field settings

Remaining tasks

patch, commit

User interface changes

no

API changes

no

CommentFileSizeAuthor
#12 interdiff.txt1.32 KBandypost
#12 2467293-vocabulary-12.patch4.31 KBandypost
#10 2467293-vocabulary-3.patch4.43 KBalx_benjamin
#8 2467293-vocabulary-2.patch4.31 KBalx_benjamin
#3 2467293-vocabulary-test-3.patch1.32 KBandypost
#1 2467293-vocabulary-1.patch2.57 KBandypost

Comments

andypost’s picture

andypost
he/him
Primary language Russian
commented
Component: taxonomy.module » entity_reference.module
Status: Active » Needs review
StatusFileSize
new 2467293-vocabulary-1.patch2.57 KB

Initial fix, not sure $field->set('settings', $settings) is right but it works

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented
Component: entity_reference.module » taxonomy.module

The fix is purely in Vocabulary.php so it belongs to taxonomy queue. I'll ping @amateescu.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Component: taxonomy.module » entity_reference.module
Issue tags: -Needs tests
StatusFileSize
new 2467293-vocabulary-test-3.patch1.32 KB

Here's a test, somehow this does not work with patch

Status: Needs review » Needs work

The last submitted patch, 3: 2467293-vocabulary-test-3.patch, failed testing.

Status: Needs work » Needs review

alx_benjamin queued 3: 2467293-vocabulary-test-3.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 3: 2467293-vocabulary-test-3.patch, failed testing.

alx_benjamin’s picture

alx_benjamin commented

The test passed on my local but failing on drupal site.
Will be looking into it more.

-------------------------------------------------
Sponsored by http://reallifedesign.co.uk/

alx_benjamin’s picture

alx_benjamin commented
Status: Needs work » Needs review
StatusFileSize
new 2467293-vocabulary-2.patch4.31 KB

We are trying to run tests on both patches separately where as they should be tested together.
So in this patch I merged both patches into one. Let's see if it will pass test this time.

-------------------------------------------------
Sponsored by http://reallifedesign.co.uk/

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

Looks good, couple of minor issues

  1. +++ b/core/modules/taxonomy/src/Entity/Vocabulary.php
@@ -135,29 +135,35 @@ public function postSave(EntityStorageInterface $storage, $update = TRUE) {
-        return $field_storage->getType() == 'entity_reference' && $field_storage->getSetting('target_type') == 'taxonomy_term';

    Any reason we lost the getType() check? 

  2. +++ b/core/modules/taxonomy/src/Entity/Vocabulary.php
@@ -135,29 +135,35 @@ public function postSave(EntityStorageInterface $storage, $update = TRUE) {
+        return $field->getFieldStorageDefinition()->getSetting('target_type') == 'taxonomy_term';
...
+          if ($value == $this->getOriginalId()) {

    nit: === instead of == 

  3. +++ b/core/modules/taxonomy/src/Entity/Vocabulary.php
@@ -135,29 +135,35 @@ public function postSave(EntityStorageInterface $storage, $update = TRUE) {
+            $value= $this->id();

    nit: needs a space before the = 

  4. +++ b/core/modules/taxonomy/src/Tests/TermEntityReferenceTest.php
@@ -79,5 +80,18 @@ function testSelectionTestVocabularyRestriction() {
+    $this->assertFalse(isset($target_bundles[$old_name]), 'Field not references old vocabulary');

    nit: 'Field does not' instead of 'Field not'

alx_benjamin’s picture

alx_benjamin commented
StatusFileSize
new 2467293-vocabulary-3.patch4.43 KB

Corrected and attached as per comment #9

-------------------------------------------------
Sponsored by http://reallifedesign.co.uk/

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs review » Needs work

other points are valid except

+++ b/core/modules/taxonomy/src/Entity/Vocabulary.php
@@ -135,29 +135,35 @@ public function postSave(EntityStorageInterface $storage, $update = TRUE) {
       $field_map = \Drupal::entityManager()->getFieldMapByFieldType('entity_reference');
...
+        return $field->getFieldStorageDefinition()->getType() == 'entity_reference' && $field->getFieldStorageDefinition()->getSetting('target_type') == 'taxonomy_term';

it's not needed! because above we get ER fields only from map

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs work » Needs review
StatusFileSize
new 2467293-vocabulary-12.patch4.31 KB
new interdiff.txt1.32 KB

Fixed patch

amateescu’s picture

amateescu commented
Status: Needs review » Closed (duplicate)

This looks like a duplicate of #1978714: Entity reference doesn't update its field settings when referenced entity bundles are deleted.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Closed (duplicate) » Needs review
Related issues: +#1978714: Entity reference doesn't update its field settings when referenced entity bundles are deleted

Introduction of more generic approach is a feature, also related issue has interdependency

I think better to fix annoying bug first that could lead to data-loss

alx_benjamin’s picture

alx_benjamin commented

I think patch 2467293-vocabulary-12.patch is a good one.
Shall we move it to 'Reviewed and tested by the community'?

-------------------------------------------------
Sponsored by http://reallifedesign.co.uk/

yched’s picture

yched commented 
+++ b/core/modules/taxonomy/src/Entity/Vocabulary.php
@@ -135,29 +136,37 @@ public function postSave(EntityStorageInterface $storage, $update = TRUE) {
+        $settings = $field->getSettings();
...
+          $settings['handler_settings']['target_bundles'] = $target_bundles;
+          $field->set('settings', $settings)->save();

using the generic ->set() is weird, why not use the dedicated getSettings() / setSettings(), or directly the more focused ->getSetting('handler_settings') / ->setSetting('handler_settings', $new_value) ?

legolasbo’s picture

legolasbo
he/him
Primary language Dutch
Location Middelburg
commented
Status: Needs review » Needs work
claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented

Why not focusing on #1978714-46: Entity reference doesn't update its field settings when referenced entity bundles are deleted? That is the generic fix and we can get that in without #2412569: Allow setting the auto-create bundle on entity reference fields with multiple target bundles.

amateescu’s picture

amateescu commented
Status: Needs work » Closed (duplicate)

#1978714: Entity reference doesn't update its field settings when referenced entity bundles are deleted is quite active at the moment and it's fixing this bug.