Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Looks similar to #1054162: Taxonomy bundles not supported by EntityFieldQuery (followup)
Attached test verifies the fail (will attach once I get a node id).
Comment | File | Size | Author |
---|---|---|---|
#32 | 1845372_32.patch | 2.37 KB | chx |
#31 | 1845372_30.patch | 8.95 KB | chx |
#26 | field-delete-non-node-1845372.26.patch | 1.37 KB | Berdir |
#24 | field-delete-non-node-1845372.24.patch | 1.37 KB | larowlan |
#18 | efq-1845372-18-tests.patch | 4.17 KB | xjm |
Comments
Comment #1
larowlanScreenshot of error in UI.
Attached is failing test to demonstrate the regression.
Comment #2
larowlanComment #4
chx CreditAttribution: chx commentedComment #5
larowlanThis is field_purge_batch, this line:
because there is no vocabulary_machine_name column in taxonomy_term
patch coming
Comment #6
larowlanPatch that adds convoluted logic for taxonomy_term, it's yuck but it works.
Comment #7
chx CreditAttribution: chx commentedwell, 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?
Comment #8
larowlanyep, 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?
Comment #9
amateescu CreditAttribution: amateescu commentedDare 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.
Comment #10
YesCT CreditAttribution: YesCT commentedcan we roll these tests into that issue and postpone this one?
Comment #11
swentel CreditAttribution: swentel commentedMarked #1848004: "QueryException: node_type not found" when deleting field from comment as a duplicate, this happens on comment fields too.
Comment #12
xjmYeah, 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.
Comment #13
xjmComment #14
YesCT CreditAttribution: YesCT commentedComment #15
xjmSTR from #1848004: "QueryException: node_type not found" when deleting field from comment:
Comment #16
xjmSo, weirdness... #12 passes, even though there's an exception onscreen in the verbose results.
Comment #17
YesCT CreditAttribution: YesCT commentedmight 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
Comment #18
xjm#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()
.Comment #20
xjmThere we go. So, a correct solution for this will make the tests in #18 pass.
Comment #21
sunClosely 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?
Comment #22
larowlan#1: field-delete-non-node-1845372.1.fail_.patch queued for re-testing.
Comment #23
larowlanRetesting, since vocabs are now config entities, the vocab based test at #1 should now pass.
Comment #24
larowlanRe-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.
Comment #26
BerdirIt'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?
Comment #27
larowlan@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?
Comment #28
chx CreditAttribution: chx commentedLet'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.
Comment #29
bojanz CreditAttribution: bojanz commentedWe'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.
Comment #30
yched CreditAttribution: yched commented1) sounds reasonable and way simpler than 2) ?
Comment #31
chx CreditAttribution: chx commentedI 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.
Comment #32
chx CreditAttribution: chx commentedSure, 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.
Comment #33
yched CreditAttribution: yched commentedThis looks good to me.
Comment #34
xjmWe should probably add test coverage using test field and entity types, I think. I should have thought of that before. :)
Comment #35
webchickIs #34 a "needs work" or a "someone should file a follow-up about this"?
Comment #36
xjmI discovered that the entire Field UI test suite uses node, never a test entity, so I think a followup is best.
Comment #37
xjmI opened #1885760: Add or convert Field UI test coverage to use a test entity instead of node.
Comment #38
webchickCool.
Committed and pushed to 8.x. Thanks!
Comment #40
yched CreditAttribution: yched commentedBumped 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:
Does anyone have an issue # in mind ? Can't seem to find it.
Comment #41
larowlan#731724: Convert comment settings into a field to make them work with CMI and non-node entities fixes this
Comment #42
yched CreditAttribution: yched commentedOK, however @Berdir in #26 seems to imply that there is another, dedicated issue for this ?
Comment #43
andypost@yched suppose it's only one issue that adds a bundle column to comment table.
Probably @Berdir could tell more
Comment #44
BerdirI 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.
Comment #45
yched CreditAttribution: yched commentedOk, thanks guys.