Looks similar to #1054162: Taxonomy bundles not supported by EntityFieldQuery (followup)

Attached test verifies the fail (will attach once I get a node id).

Files: 
CommentFileSizeAuthor
#32 1845372_32.patch2.37 KBchx
PASSED: [[SimpleTest]]: [MySQL] 50,673 pass(es).
[ View ]
#31 1845372_30.patch8.95 KBchx
PASSED: [[SimpleTest]]: [MySQL] 50,616 pass(es).
[ View ]
#26 field-delete-non-node-1845372.26.patch1.37 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 50,136 pass(es).
[ View ]
#24 field-delete-non-node-1845372.24.patch1.37 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] 50,075 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#18 efq-1845372-18-tests.patch4.17 KBxjm
FAILED: [[SimpleTest]]: [MySQL] 48,802 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#18 efq-1845372-18.patch5.34 KBxjm
FAILED: [[SimpleTest]]: [MySQL] 48,831 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#18 interdiff.txt1.46 KBxjm
#12 efq-1845372-comment-test.patch1.91 KBxjm
Test request sent.
[ View ]
#12 efq-1845372-11-combined.patch4.53 KBxjm
Test request sent.
[ View ]
#6 field-delete-non-node-1845372.6.interdiff.txt1.88 KBlarowlan
#6 field-delete-non-node-1845372.6.patch2.62 KBlarowlan
PASSED: [[SimpleTest]]: [MySQL] 48,319 pass(es).
[ View ]
#1 Screen Shot 2012-11-20 at 1.20.00 PM.png45.33 KBlarowlan
#1 field-delete-non-node-1845372.1.fail_.patch1.45 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch field-delete-non-node-1845372.1.fail_.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Comments

StatusFileSize
new1.45 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch field-delete-non-node-1845372.1.fail_.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new45.33 KB

Screenshot of error in UI.
Screen Shot 2012-11-20 at 1.20.00 PM.png

Attached is failing test to demonstrate the regression.

Status:Active» Needs review

Status:Needs review» Needs work

The last submitted patch, field-delete-non-node-1845372.1.fail_.patch, failed testing.

Assigned:Unassigned» chx

Title:Deleting a field from other than a node entity results in an Entity Field Query ExceptionDeleting a field from a taxonomy_term results in an Entity Field Query Exception

This is field_purge_batch, this line:

<?php
$query
->condition($info[$entity_type]['entity_keys']['bundle'], $ids->bundle);
?>

because there is no vocabulary_machine_name column in taxonomy_term

patch coming

Status:Needs work» Needs review
StatusFileSize
new2.62 KB
PASSED: [[SimpleTest]]: [MySQL] 48,319 pass(es).
[ View ]
new1.88 KB

Patch that adds convoluted logic for taxonomy_term, it's yuck but it works.

well, you admitted it being yuck. Can't we add a drupal_alter to the entity query base, at execute time , and besides the 'entity_query' alter , add an "entity_query_$entity_type" alter, let taxonomy implement it and get this fixed for all queries thusly?

yep, the entity_query_$entity_type alter is already in the execute, but from memory this blows up in the compile(), is the alter too later by then?

Dare I say that the best fix for this issue is actually getting #1552396: Convert vocabularies into configuration RTBC and committed? :) The test additions are useful though.

can we roll these tests into that issue and postpone this one?

Marked #1848004: "QueryException: node_type not found" when deleting field from comment as a duplicate, this happens on comment fields too.

StatusFileSize
new4.53 KB
Test request sent.
[ View ]
new1.91 KB
Test request sent.
[ View ]

Yeah, it is not a duplicate of the vocab conversion because it happens with any non-node content entity.

Here's an additional test for comments, which will fail. So I think we need a more generic fix.

Edit: Really the Field UI tests would be better using their test entity type, but the field management UI doesn't seem to be exposed for that entity yet.

Title:Deleting a field from a taxonomy_term results in an Entity Field Query ExceptionDeleting a field from a non-node entity bundle results in an Entity Field Query Exception

STR from #1848004: "QueryException: node_type not found" when deleting field from comment:

Steps to reproduce:

  1. Install 8.x with the standard profile.
  2. Go to admin/structure/types/manage/article/comment/fields.
  3. Delete the comment body field. The field is deleted, but you will see the exception:
    Drupal\Core\Entity\Query\QueryException: node_type not found in Drupal\field_sql_storage\Entity\Tables->ensureEntityTable() (line 100 of /.../core/modules/field/modules/field_sql_storage/lib/Drupal/field_sql_storage/Entity/Tables.php

The bug also occurs with field instances on taxonomy vocabularies, but not with fields on node types. ManageFieldsTest does not catch it because it uses node rather than a test entity.

So, weirdness... #12 passes, even though there's an exception onscreen in the verbose results.

might be worth retesting (uploading the same patches again). There was some testbot weirdness then. #1848714: testbot not reporting results back to issue queues, no Re-test link

Issue tags:-regression
StatusFileSize
new1.46 KB
new5.34 KB
FAILED: [[SimpleTest]]: [MySQL] 48,831 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new4.17 KB
FAILED: [[SimpleTest]]: [MySQL] 48,802 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

#17 is not the cause of the weirdness in this case. As I stated, they passed on PIFR (click the "view details" link to see) as well as locally when they should have failed, unrelated to the PIFT problem. :)

The problem is that $this->drupalGet() doesn't throw a fail on an exception in the child site.

Attached attempts to work around this with an assertResponse().

Status:Needs review» Needs work

The last submitted patch, efq-1845372-18.patch, failed testing.

There we go. So, a correct solution for this will make the tests in #18 pass.

Closely related:

The new EFQ code also makes the assumption that all entities would be stored locally and thus that they would have a 'base_table' to query/join on.

That assumption did not exist before - or at least, the old EFQ did not throw an exception when Field API tried to look up field values in various ways (i.e., querying the field data tables, but skipping over the entity base table, as it does not exist). I don't know how remote entities can work with that. I'm facing these problems over in #1825044: Turn contact form submissions into full-blown Contact Message entities, without storage.

Is this something that can be fixed as part of this issue or should I create a separate one?

Status:Needs work» Needs review

Retesting, since vocabs are now config entities, the vocab based test at #1 should now pass.

StatusFileSize
new1.37 KB
FAILED: [[SimpleTest]]: [MySQL] 50,075 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Re-rolling patch from #1 against HEAD, patch just adds tests for deleting non-node entities (taxonomy in this case).
If this passes, I think its good to go in - all it adds is tests so we don't get another regression. As per #9, the actual cause of the issue was resolved with #1552396: Convert vocabularies into configuration.
@sun re #21, I think its a new issue.

Status:Needs review» Needs work

The last submitted patch, field-delete-non-node-1845372.24.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.37 KB
PASSED: [[SimpleTest]]: [MySQL] 50,136 pass(es).
[ View ]

It's 'Tags', not 'tags' :)

Also, the confirmation message is nonsense ("The field Test has been deleted from the Tags content type.", content type, really?) but that's not our problem.

Also, I think we lost the comment test coverage from @xjm, which will still fail. I don't think there is a generic fix to it, entity types just have to add their fricking bundle column to the table or query alter it in. And there's an issue to add the bundle to the comment table. Maybe the comment refactoring issue from @larowlan fixes it too?

Assigned:chx» Unassigned

@berdir thanks, I'd picked up the tags/Tags in #6 but had rolled my local branch back to the patch at 1
Yeah the refactor adds field name (which is bundle) to the comment table.
Are we happy to add the comment tests at #17 in over there and move forward with the patch at 26?

Status:Needs review» Reviewed & tested by the community

Let's do this. I showed this issue to bojanz and his reaction (mine too) was "if your entity type has a base table, it needs to have a bundle key that is not computed" which is exactly what #26/#27 suggests.

Status:Reviewed & tested by the community» Needs review

We've been discussing this some more.

There are two cases here:
1) Broken core entities that declare a bundle key but don't store it in the base table
2) Entities without a bundle key (meaning $bundle == $entity_type)

We don't need to care about #1. But we need to care about #2 (otherwise we'd need to store the bundle in the base table even if it never changes).

Two solutions discussed were:
1) Add a check around the bundle condition, only adding it if there is a bundle key. Something like http://privatepaste.com/6af20e66ea
2) Changing field_purge_batch() not to filter by bundle at all, dropping $instance from field_purge_data($entity, $field, $instance), adding a new field storage method to lookup bundles based on ids. chx can explain this more.

1) sounds reasonable and way simpler than 2) ?

StatusFileSize
new8.95 KB
PASSED: [[SimpleTest]]: [MySQL] 50,616 pass(es).
[ View ]

I rewrote the function. It now selects a bunch of entities for a given field and entity_type and then asks the storage engine (new storage hook) what bundles do these entities belong to? The field storage engine knows this even if the query engine doesn't. Magic! :) I added #17 and #26 and those and the existing field bulk delete test passes.

StatusFileSize
new2.37 KB
PASSED: [[SimpleTest]]: [MySQL] 50,673 pass(es).
[ View ]

Sure, we can do this, it's #26 and #29 fixed together. I am fine with this too. It's cleaner than #31, solves user (single bundle), if larowlan solves comments, then we are golden.

Status:Needs review» Reviewed & tested by the community

This looks good to me.

We should probably add test coverage using test field and entity types, I think. I should have thought of that before. :)

Status:Reviewed & tested by the community» Needs review

Is #34 a "needs work" or a "someone should file a follow-up about this"?

Status:Needs review» Reviewed & tested by the community

I discovered that the entire Field UI test suite uses node, never a test entity, so I think a followup is best.

Status:Reviewed & tested by the community» Fixed

Cool.

Committed and pushed to 8.x. Thanks!

Status:Fixed» Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

Bumped on this while working on test fails for the "Field API / CMI" patch. field_purge_batch() dies when trying to purge field data on a comment entity.

From @Berdir #26:

entity types just have to add their fricking bundle column to the table or query alter it in. And there's an issue to add the bundle to the comment table.

Does anyone have an issue # in mind ? Can't seem to find it.

OK, however @Berdir in #26 seems to imply that there is another, dedicated issue for this ?

@yched suppose it's only one issue that adds a bundle column to comment table.
Probably @Berdir could tell more

I think there is or was a specific issue for that but it's only relevant if #731724: Convert comment settings into a field to make them work with CMI and non-node entities would not make it in. No need to unecessarly conflict with that issue otherwise as it is done in a different way anyway there.

Ok, thanks guys.