Deleting a taxonomy vocabulary leaves term reference fields still pointing to it, and a PDO Exception when creating content

The vocabulary API just isn't using the Field API like it ought to. Adding the field_create_field() and field_delete_field() calls is pretty easy, but there's no UI to set the field settings from the vocabulary form. Personally, I think that's a separate issue, and should be easy enough to fix.

There's a similar problem with node types, but let's keep things simple for now.

I guess I was wrong about the bundle name needing to change. Hopefully HEAD is better now.

Aha. I found the specific error message, and these changes made the standard profile a little out of date.

#5: 687180_taxonomy_node_fields.patch queued for re-testing.

Just found #628244: No way to attach the automatically created taxo field, which explains why it works this way. So, whenever you delete a vocabulary, you have to remember to delete it from all of the bundles it's been attached to.

No thank you. When a vocabulary is deleted, its related term reference field should be deleted because it can't hold valid data any more.

I understand needing to create the field when it is first attached to a bundle, though. You have to do that to figure out which node types apply the vocabulary applies to, anyway.

Seriously, I can't even get a one-line patch right?

...OK. Now the tests should work.

#13: 687180_vocabulary_delete_field.patch queued for re-testing.

This is a more serious bug than it looks like, and easy to reproduce. All you have to do is this:

1. Install Drupal with the standard profile.
2. Delete the "Tags" vocabulary via the UI.
3. Create an article, notice that the Tags field is still there, and type something into it. When you try to save the article you wind up with a whitescreen and the following PDO Exception:

PDOException: SQLSTATE[42000]: Syntax error or access violation: 1064 You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near '))' at line 2: SELECT base.tid AS tid, base.vid AS vid, base.name AS name, base.description AS description, base.format AS format, base.weight AS weight, v.machine_name AS vocabulary_machine_name FROM {taxonomy_term_data} base INNER JOIN {taxonomy_vocabulary} v ON base.vid = v.vid WHERE (base.name LIKE :db_condition_placeholder_0 ESCAPE '\\') AND (base.vid IN ()) ; Array ( [:db_condition_placeholder_0] => test ) in DrupalDefaultEntityController->load() (line 196 of /home/droth/web/drupal/includes/entity.inc).

Confirmed.

The problem with the existing patches was they they assumed the field names, but that's wrong. taxonomy_tags is just what's used for the default tags field, fields created through the UI are prefixed with field and can have any name.

We need to load all taxonomy fields. loop over them to see if they're using this vocabulary and if yes, delete them. What I *don't* understand is why the allowed_values structure is an array, it's not possible to assign a single field with multiple vocabularies, is it? (At least not through the UI... )

Added a simple test, the test fails without the fix.

I actually did this inside the API function, that also removes bundles and stuff so I think that's ok. While doing that, I noticed that there is a clear_clear_all() call in there too, so there is no need for the one the form submit function.

@Berdir: it's possible via the API to have a single field with multiple vocabularies yes. I don't think there's a UI for this yet, but it lets you do things like have structured vocabularies but a single autocomplete widget on the node form.

Patch looks good, but if we're removing one of the cache_clear_all(), could we remove it from taxonomy_vocabulary_delete() instead and leave it in the submit handler? - node_save() and comment_save() already did that in D7. Vocabulary deletion is pretty rare so it doesn't matter much but good to have this consistent.

Sure, I can delete it inside the API function, just wondering if that is a good idea at this point because it would change what that function is doing, in case someone is using it.

About the multiple vocabularies thing, what is supposed to happen when you have a field that points to two vocabularies and then delete one of them? Delete the field or just remove the reference to that vocab?

That's a good question and I'm not sure. When this kind of thing comes up, I'm usually more on the side of "Don't let people delete stuff that's tied into live configuration that their site might be relying on", but apparently a lot of people don't agree with me on that. Node reference iirc allows you to set up references only to particular content types, I wonder if and how that deals with node types getting deleted.

Overall I think we really need to look at bundles again in Drupal 8 because it's very confusing what the expectations are from the API.

Folks,

I read your thread and it looks like I have been the victim of what you are discussing. I deleted a vocabulary not thinking that would wipe out my stories but it happened. The nodes are still safe in the DB and the titles are showing up on the front page linking to pages not found.

I have been trying to reconnect the new vocabulary to these "lost" stories via the database but to no avail.

Any suggestions how I could recover these stories?

the site is ellatinopost.com

Thanks much.

.pablo

Moving to the 8.x queue and tagging for possible backport.

I'm not convinced we should delete a field just because it has a vocabulary referenced as an allowed value. It's feasible for a field to be allowed to reference more than one vocabulary, so what if there is valid content referencing the other vocabulary? Would rather prevent deletion of the vocabulary and instruct people to change the field settings first.

Had another look at this - since deleting a vocabulary deletes the terms, we can safely remove these fields, so it is a case of figuring out if the field is multi-reference or not - if it's multi-reference, we can nuke the allowed values key and save the field back again instead of deleting.

Patch is untested.

Well I did test it for syntax errors, just not the actual patch that got uploaded, grr.

The error is:

PDOException: SQLSTATE[42000]: Syntax error or access violation: 1305 SAVEPOINT savepoint_1 does not exist in field_read_instances() (line 703 of /var/lib/drupaltestbot/sites/default/files/checkout/modules/field/field.crud.inc).

Which is this bit:

 $results = $query->execute();

  foreach ($results as $record) {

I don't believe this patch could have caused that, so requesting retest.

Hah, reproduced locally.

This looks like #1007830: Nested transactions throw exceptions when they got out of scope, which is supposed to be fixed.

This patch should still fail, but would like to record that properly now that the main transaction overhaul patch is in.

#26: test_and_fix.patch queued for re-testing.

Yep, same error as http://drupal.org/node/687180#comment-4694714

Marking this as postponed on #1007830: Nested transactions throw exceptions when they got out of scope.

hmm postponed doesn't work for that either, 'cos that's equivalent to hiding it at the moment.

Tagging.

Subscribe. This issue breaks http://drupal.org/project/filefield_paths paths don't generate correctly after a vocabulary has been deleted. I had fields in the vocabulary, so had to delete entries in field_config_instance also.

subscribe

Here's a re-roll of #26, plus the latest patch from #1007830: Nested transactions throw exceptions when they got out of scope for the bot.

Bot error was:

PDOException: SQLSTATE[42000]: Syntax error or access violation: 1305 SAVEPOINT savepoint_1 does not exist in field_read_instances() (line 703 of /var/lib/drupaltestbot/sites/default/files/checkout/modules/field/field.crud.inc).

Retesting because that is kind of weird.

#39: taxonomy_field.patch queued for re-testing.

See #1007830-107: Nested transactions throw exceptions when they got out of scope for the remaining issue we have around transactions and DDL.

About this issue: let's not call field_update_field() when it's not necessary. If the field has no data, this triggers a DROP TABLE / CREATE TABLE sequence which is the one that messes up the transaction stack. Without this, I'm pretty sure that this patch can pass.

See also #1065814: Content remains tagged with a taxonomy term (via the {taxonomy_index} table) even after its corresponding term reference field is deleted and purged. I've requested in that issue that someone test to see if this patch resolves it as well.

Edit: and #89181: Use queue API for node and comment, user, node multiple deletes.

Related: #1363200: Term reference fields can be created with no vocabulary, another way to end up with a term reference field without a valid vocabulary.

(d7 Reference issue, but with deletion of nodes/users: #1346696: On deletion of node/user, references to it are not removed #589714: Nodereference filtering: invalid reference should be same as no reference)

Untagging.

#44: 687180-taxonomy-field-cleanup.patch queued for re-testing.

So this looks fine now, I'm wondering if we should add an additional test for #18 - i.e. create a field with two vocabularies, delete one of them, make sure the settings are correct afterwards.

Sounds like a good idea to me. I'd also like to see if the patch here resolves #1065814: Content remains tagged with a taxonomy term (via the {taxonomy_index} table) even after its corresponding term reference field is deleted and purged, and if so, add a test for that bug as well.

So aLearner confirmed that the patch here does not resolve #1065814: Content remains tagged with a taxonomy term (via the {taxonomy_index} table) even after its corresponding term reference field is deleted and purged, so we just need an additional test along the lines catch describes in #50. I'll work on that.

Attached:

• Adds test coverage for a field that uses two vocabularies.
• Removes t() from an assertion message. (Reference: #500866: [META] remove t() from assert message)
• Clarifies a comment and capitalizes "Taxonomy module." (Reference: http://drupal.org/node/604342)

Looks good to me.

Committed/pushed to 8.x, moving back to 7.x for backport.

Failed to actually move this to 7.x.

Should be a straightforward backport, tagging novice.

Back-porting #53.

D7 backport of #53.

Changed status.

Backport matches up, passes tests, and doesn't include any D8-specific code => RTBC. Thanks @anthbel!

Looks good. Committed and pushed to 7.x. Thanks!

Related:
#1778572: EntityMalformedException: Missing bundle property on entity of type node. in entity_extract_ids() (line 7562

Beginning to write summary.