Support from Acquia helps fund testing for Drupal Acquia logo

Comments

larowlan’s picture

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

Attached is failing test to demonstrate the regression.

larowlan’s picture

Status: Active » Needs review

Status: Needs review » Needs work

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

chx’s picture

Assigned: Unassigned » chx
larowlan’s picture

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

This is field_purge_batch, this line:

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

because there is no vocabulary_machine_name column in taxonomy_term

patch coming

larowlan’s picture

Status: Needs work » Needs review
FileSize
2.62 KB
1.88 KB

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

chx’s picture

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?

larowlan’s picture

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?

amateescu’s picture

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.

YesCT’s picture

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

swentel’s picture

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

xjm’s picture

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.

xjm’s picture

Title: Deleting a field from a taxonomy_term results in an Entity Field Query Exception » Deleting a field from a non-node entity bundle results in an Entity Field Query Exception
YesCT’s picture

xjm’s picture

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.

xjm’s picture

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

YesCT’s picture

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

xjm’s picture

Issue tags: -Regression
FileSize
1.46 KB
5.34 KB
4.17 KB

#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.

xjm’s picture

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

sun’s picture

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?

larowlan’s picture

Status: Needs work » Needs review
larowlan’s picture

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

larowlan’s picture

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.

Berdir’s picture

Status: Needs work » Needs review
FileSize
1.37 KB

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?

larowlan’s picture

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?

chx’s picture

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.

bojanz’s picture

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.

yched’s picture

1) sounds reasonable and way simpler than 2) ?

chx’s picture

FileSize
8.95 KB

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.

chx’s picture

FileSize
2.37 KB

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.

yched’s picture

Status: Needs review » Reviewed & tested by the community

This looks good to me.

xjm’s picture

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

webchick’s picture

Status: Reviewed & tested by the community » Needs review

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

xjm’s picture

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.

xjm’s picture

webchick’s picture

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.

yched’s picture

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.

larowlan’s picture

yched’s picture

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

andypost’s picture

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

Berdir’s picture

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.

yched’s picture

Ok, thanks guys.