Importing taxonomy vocabulary config throws PHP notices and Warning

[jenlampton]

Steps to reproduce:

1. Create a new taxonomy vocabulary on one site
2. Export config for full site
3. import config for full site (on another site that does not have the vocabulary)
4. see the error, as below:

Notice: Undefined offset: 1 in taxonomy_config_create_validate() (line 1767 of backdrop/core/modules/taxonomy/taxonomy.module).
Notice: Undefined offset: 1 in taxonomy_config_create_validate() (line 1767 of backdrop/core/modules/taxonomy/taxonomy.module).
Notice: Undefined offset: 1 in taxonomy_config_create_validate() (line 1767 of backdrop/core/modules/taxonomy/taxonomy.module).
Notice: Undefined offset: 1 in taxonomy_config_create_validate() (line 1767 of backdrop/core/modules/taxonomy/taxonomy.module).
Notice: Undefined offset: 1 in taxonomy_config_create_validate() (line 1767 of backdrop/core/modules/taxonomy/taxonomy.module).
Notice: Undefined offset: 1 in taxonomy_config_create_validate() (line 1767 of backdrop/core/modules/taxonomy/taxonomy.module).

[yorkshire-pudding]

I suspect this is related: Same steps to reproduce but now get warning instead. This was using bee to import config. [!] Warning: Undefined array key 1 in taxonomy_config_create_validate() (line 2169 of home/user/website/live/docroot/core/modules/taxonomy/taxonomy.module). Config is still imported.

Backdrop version: 1.27.1

[yorkshire-pudding]

This is the line: https://github.com/backdrop/backdrop/blob/8352696d70bfff4f1d3e4b8f56c29e36b6688f18/core/modules/taxonomy/taxonomy.module#L2169

    list($vocabulary_name, $field_name) = explode('.', str_replace('taxonomy.vocabulary.', '', $config_name));

In my case the $config_name was taxonomy.vocabulary.other_writing_type and I therefore think that explode() is the culprit here; once taxonomy.vocabulary is replaced with '' there is nothing to explode using . but it looks like this was maybe done to support fields in the vocabulary so maybe the error occurs if there are no fields defined or the vocab entity itself.

Any thoughts @jenlampton ?

[yorkshire-pudding]

Got this again today. Looking at the function: taxonomy_config_create_validate(), the purpose of this is to ensure that if the config is taxonomy vocab fields then the taxonomy vocab must exist first. Therefore, a simple check to see if this is a vocab field config before doing the rest should fix; the check is not applicable to importing the vocabulary itself.

I've added a PR There's a cspell failure unrelated to the bit of the file I've changed, but all other checks pass.

@jenlampton - would you care to test/review? @avpaderno - as you've reacted, perhaps you have an interest and would be willing to test/review?

[avpaderno]

I created a new vocabulary with some taxonomy terms, I did a full export, which I then imported in the PR. I did not get any warning.

[avpaderno]

I did the same operations in the opposite direction, from this PR to another PR. In this case, I got a Warning: Undefined array key 1 in taxonomy_config_create_validate() (line 2169 of /var/lib/tugboat/core/modules/taxonomy/taxonomy.module). I did not get in the first case.

(I will try again importing in this PR, to be sure I did all correctly.)

[avpaderno]

I tried again importing a full configuration containing a new vocabulary from another PR to this PR. I can confirm I did not get any warning. The only message was Configuration sync completed. 2 configuration files synced.

[avpaderno]

I hope I correctly changed the labels.

[yorkshire-pudding]

Thanks @avpaderno

[avpaderno]

I apologize: I verified the PR would not raise any warning, but that is probably not sufficient.

Since I exported a configuration with a new vocabulary and new terms, I would expect the imported vocabulary to contain those terms. What I read in the PR, on admin/structure/taxonomy/list, under Number of terms, is 0.

[avpaderno]

(To make it clear: That is also what happens when I export from this PR and import in another PR.)

[argiepiano]

Since I exported a configuration with a new vocabulary and new terms, I would expect the imported vocabulary to contain those terms.

I think there is a misunderstanding of what a vocabulary is. A vocabulary is the equivalent of a content type. When you export a content type you don't get the content itself, only the content type configuration. Vocabularies work the same way. You don't get the terms of the vocabulary, only the vocab configuration, when you import it.

[yorkshire-pudding]

@avpaderno - I don't think terms are exported and imported anyway. The vocab and vocab config are, but not the content, which go into the database.

Personally, I find this an annoyance when I use taxonomy to provide options in a node form but I don't think there is any mechanism to import the terms (yet). I imagine, while it would be simple for vocab without any extra fields, once you start adding additional fields it gets complicated.

[avpaderno]

I was expecting the taxonomy terms to be imported too because I considered them the allowed values for that vocabulary.
The vocabulary is imported without warning, so the PR is doing its work.

[yorkshire-pudding]

Would anyone be able to add a milestone candidate - bug label?

[argiepiano]

To me, this looks like a copy-paste problem in taxonomy_config_create_validate(). This was probably copied from field_config_create_validate(), node_config_create_validate() or some other implementation.

The thing with vocabularies is that, unlike field instances or field info configurations, they never will have more than two periods in their name. They always follow the pattern taxonomy.vocabulary.MACHINE_NAME. There will never be anything more to extract (the node and field implementations do need to extract the field name to verify that the field actually exists, for example). So checking whether the vocabulary exists is completely unnecessary, since you are effectively importing a vocabulary that doesn't exist.. The whole hook can actually be removed. It's not needed.

[avpaderno]

Looking at node_config_create_validate(), which uses the following code, I wonder if the intent was for taxonomy_config_create_validate() to verify the vocabulary for the imported vocabulary fields exists.

$config_name = $staging_config->getName();

// Ensure entity types and bundles exist or will be imported.
if (strpos($config_name, 'field.instance.node.') === 0) {
  list($node_type, $field_name) = explode('.', str_replace('field.instance.node.', '', $config_name));

// Check that the node type does or will exist.
$type_exists = (bool) node_type_get_type($node_type);
    $type_created = $all_changes['node.type.' . $node_type];
    if (!$type_exists && !$type_created) {
      throw new ConfigValidateException(t('The field "@name" cannot be added because the node type "@type" does not exist.', array('@name' => $field_name, '@type' => $node_type)));
    }
  }

In that case, what is wrong with the code used by taxonomy_config_create_validate() is the 'taxonomy.vocabulary.' string.

As evidence that taxonomy_config_create_validate() is probably trying to verify the imported vocabulary fields are for an existing vocabulary: The thrown exception says The field "@name" cannot be added because the vocabulary "@vocabulary" does not exist.

[avpaderno]

The PR does not show any warning because the code causing the warning will never be executed.

// Ensure the taxonomy vocabulary exists before importing fields. First check
// this is a config file for a taxonomy vocabulary field.
if (strpos($config_name, 'taxonomy.vocabulary.') === 0 && substr_count($config_name, '.') > 2) {
  list($vocabulary_name, $field_name) = explode('.', str_replace('taxonomy.vocabulary.', '', $config_name));

  // Check that the vocabulary does or will exist.
  $vocabulary_exists = (bool) taxonomy_vocabulary_load($vocabulary_name);
  $vocabulary_created = $all_changes['taxonomy.vocabulary.' . $vocabulary_name];
  if (!$vocabulary_exists && !$vocabulary_created) {
    throw new ConfigValidateException(t('The field "@name" cannot be added because the vocabulary "@vocabulary" does not exist.', array('@name' => $field_name, '@vocabulary' => $vocabulary_name)));
  }
}

substr_count($config_name, '.') > 2 will never be true, as @argiepiano said.

[quicksketch]

I wonder if the intent was for taxonomy_config_create_validate() to verify the vocabulary for the imported vocabulary fields exists.

I believe @avpaderno is correct. The if statement should probably be changed from:

  if (strpos($config_name, 'taxonomy.vocabulary.') === 0 && substr_count($config_name, '.') > 2) {
    list($vocabulary_name, $field_name) = explode('.', str_replace('taxonomy.vocabulary.', '', $config_name));

to

  if (strpos($config_name, 'field.instance.taxonomy.') === 0) {
    list($vocabulary_name, $field_name) = explode('.', str_replace('field.instance.taxonomy.', '', $config_name));

I think this was a copy/paste blunder like @argiepiano suggests.

[yorkshire-pudding]

Thanks for the feedback. I have updated the PR and this now appears to work in the different scenarios. I also found a separate bug #6715 that appears when taxonomy with field is successfully imported.

[quicksketch]

Confirmed the new validation code works as expected. I added a vocabulary, added a field to it. Exported the config. Then removed the vocabulary config file but left the field config file. I got the validation error expected that you can't import a field for a non-existent vocabulary:

I tried putting the config file back (which worked as expected), and deleting the field only and re-importing (adding the field to an existing vocab). That worked as expected too. Seems like this is all set and working as expected.

[quicksketch]

I merged https://github.com/backdrop/backdrop/pull/4871 into 1.x and 1.29.x. Thank you @yorkshire-pudding, @avpaderno, and @jenlampton!