'machine_name' is a unique key in the {taxonomy_vocabulary} table. It is therefore illegal to attempt to save a new record in the table with the same value of 'machine_name' as an existing record, and a fatal database error will occur.

taxonomy_vocabulary_save() checks for a record with the same value of vid, but it is possible that a module will attempt to save a vocabulary that has the same machine name as an existing one, without specifying a vid. A case in point is described in #972264: Unable to install in installation profile because taxonomy is creating field after media_browser_plus. Shouldn't it therefore, if $vocabulary->vid is undefined but $vocabulary->machine_name is set, attempt to find the vid for a vocabulary with that machine name, before risking saving a new one and suffering the fatal error? It certainly would be worth the small overhead to reduce errors in things like profile installations.

Happy to write a patch if anyone agrees with this.

CommentFileSizeAuthor
#3 1357420-1.patch1.35 KBjibran
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Version: 7.9 » 8.x-dev

So, the form validator for vocabulary forms checks the machine name with taxonomy_vocabulary_machine_name_load() as the exists element validation callback. taxonomy_vocabulary_save() doesn't actually check for an existing vocabulary with the vid--it just updates the existing record with that vid, because it's the primary key.

In the API, the burden is usually on the caller to pass in valid data. However, the point about installation profiles is also relevant. Maybe we should look at how machine names are validated (or not) in other CRUD API for things that have them? There are machine name fields in core for:

  1. content types
  2. vocabularies
  3. menus
  4. input formats
  5. date types
xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented

For D8, this issue will probably depend on how we manage configurable machine names and uuids in CMI. I don't think backporting a change to this would be a good idea for D7; it seems like too much of an API change.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Issue summary: View changes

Improved formatting.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented
Title: taxonomy_vocabulary_save does not check for existing machine_name » Vocabulary::save does not check for existing machine_name
Issue summary: View changes
Status: Active » Needs review
FileSize
1357420-1.patch1.35 KB

taxonomy_vocabulary_save() is Vocabulary::save() now let's try a quick test. If test fails it means the issue is fixed.

Status: Needs review » Needs work

The last submitted patch, 3: 1357420-1.patch, failed testing.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented
Title: Vocabulary::save does not check for existing machine_name » taxonomy_vocabulary_save does not check for existing machine_name
Version: 8.0.x-dev » 7.x-dev
Status: Needs work » Active

The test failed which mean this issue is fixed in D8. I don't think this test is worth adding to core. Moving it back to D7 queue.