[DX] Using taxonomy_term_save() programmatically with an invalid vocabulary results in "orphan" terms

[laz0rama]

Description of the bug

When a taxonomy term is created with a vocabulary that does not exist, the term is created with no error, but there is no way to access it through the Backdrop tools - is an orphaned row in the taxonomy_term_data table.

Steps To Reproduce

To reproduce the behavior:

$term = entity_create('taxonomy_term', array(
      'vocabulary' => $bogusMachineName,
      'name' => $term,
      'description' => $description,
      'format' => 'filtered_html',
    ));
$res = taxonomy_term_save($term);

Actual behavior

The above code runs with no problem, no errors produced. Results in an orphaned (and useless) row in the taxonomy_term_data table.

Expected behavior

The call to taxonomy_term_save() should fail with an error indicating that the specified voocabulary does not exist.

Additional information

Backdrop version 1.24.1

[klonos]

Thanks for opening this issue @laz0rama 🙏🏼

I believe that this is the respective issue in Drupal core: https://www.drupal.org/project/drupal/issues/2994657 ...it was closed as a duplicate of https://www.drupal.org/project/drupal/issues/2452267.

Also related: https://www.drupal.org/project/drupal/issues/1896286

And also related and interesting:

• https://www.drupal.org/node/1281114
• https://www.drupal.org/project/taxonomy_orphanage

[argiepiano]

This is an interesting problem - thanks for reporting @laz0rama and for finding those respective issues, @klonos.

I think the general consensus in D7 was that, if you create entities programmatically, you should take care of validating the bundle yourself (in this case, making sure the vocabulary exists - which is the bundle for taxonomy terms). The fact that those issues exist in Drupal and have not been fixed leads me to believe this is still the case.

This problem doesn't exist if you use the UI to create entities (e.g. nodes or terms). The UI forms prevent this.

[klonos]

I think the general consensus in D7 was that, if you create entities programmatically, you should take care of validating the bundle yourself ...

Yup 👍🏼

How would we handle this if we were to address it? I see that the possible return values for taxonomy_term_save() currently are:

Either SAVED_NEW or SAVED_UPDATED depending on the operation performed.

Would we introduce a new SAVE_ERROR return value or similar (and respective constant), and handle this scenario? In general, what is the proposed solution here?

[avpaderno]

Actually, there is no validation of the values used to construct an entity. Every entity class uses or extends Entity::__construct(), which contains the following code.

  // Set initial values.
  foreach ($values as $key => $value) {
    $this->$key = $value;
  }

That would be a problem with PHP 8, since PHP would report the class is using dynamic properties.

[avpaderno]

As a side note, the expectations on Drupal 8/9 are different: Entity classes have a validate() method that is supposed to validate entities. In Drupal 7 and Backdrop, that method does not exist. A Drupal 8 module could be expected to also validate the entity, which is done by calling that method; for a Backdrop / Drupal 7 module, that is not so simple.

[avpaderno]

As a further side note, taxonomy_term_save not creating with vocab machine name has been closed as works as designed, but none of the Drupal 7 maintainers took that decision. IMO, that issue has been closed too quickly, probably "nitpicking" on which of the values passed to taxonomy_term_save() is considered optional and which not.

[laz0rama]

In general, what is the proposed solution here?

i don't think my proposed solution would be very well received, at least based on some interactions i've had. my perspective is that:

a) vocabularies should have remained in the database, and b) referential integrity should be enforced at the database level, and the application should be prepared to deal with related errors as they see fit

but i am very old school when it comes to distributed applications (and how tightly the different layers are coupled), and my involvement in development these days is fairly minimal, so my opinion should not be much of a factor to those doing the actual work. who by the way, i appreciate a lot!

[klonos]

...so my opinion should not be much of a factor to those doing the actual work. who by the way, i appreciate a lot!

Open source ❤️

vocabularies should have remained in the database

Yeah, a bit too late to revert that now 😅 ...wondering if we could be throwing an exception when this happens 🤔

... the expectations on Drupal 8/9 are different: Entity classes have a validate() method that is supposed to validate entities

@kiamlaluno I think that that is a valid feature request to add to Backdrop. Do you mind opening a separate issue for it?

[avpaderno]

@klonos I am thinking about how to sell that feature request. Because it makes things easier for developers is probably not a sufficient reason to convince that entities should have a validate() method.

[klonos]

Because it makes things easier for developers is probably not a sufficient reason to convince that entities should have a validate() method.

It actually is 🙂 ...that's why this issue here (as well as the new one to create) have the [DX] prefix. Having to write less code, or duplicate less code and implement validation in a more consistent way sounds like a great improvement for developers. It will certainly help both contrib/custom modules, as well as core code.

[avpaderno]

I created #6061.