Problem/Motivation
Why issue is filed:
EntityFieldQuery (EFQ) processing is not consistent and will cause confusion.
Example / Reproduction:
- if EFQ has a FieldCondition, then node_access_grants is checked
- if EFQ has only EntityCondition, NO node_access_grant check occurs
Root cause(s):
field_sql_storage_field_storage_query() hardcodes an 'entity_field_access' tag on the query, which then triggers _node_query_node_access_alter().
Considerations while implementing solution:
- Impact to the Field UI vi field_has_data(). Why, you ask? field_has_data() relies on EFQ
- When implementing the EFQ consistency fix, See related issue with tested patches and recreation steps to test if the fix will break field_has_data() logic.: http://drupal.org/node/1302228
- entity.inc may need to check if node or not. This may be separate issue from entity field access consistency
Proposed resolution
Proposed HOTFIX:
Add 'account' in metadata. See comment: http://drupal.org/node/997394#comment-5096664
Proposed FULL SOLUTION:
Rationale: Create a consistent and flexible processing logic for EntityFieldQuery and its interaction with node_access. Benefit: it will provide a predictable interface for other operations leveraging the EFQ logic.
The solution proposal includes the following changes:
- Remove hardcoded node_access check in EFQ
- Support the node_access check in EFQ. It is up to the caller add the tag
- The tag should be removed from field_sql_storage_field_storage_query()
- Support for the tag should be added for EFQs without field conditions (does this need to be supported for all cases? w w/o field conditions.
Remaining tasks
- cradle to grave impact analysis of *where* the fix needs to happen and what may break when implementing solution
- identify the interaction behavior between entity.inc and EFQ. define if any work to entity.inc needs to be added in the proposed solution
- identify proposed solution, review with those on the comment list for feedback.
- fix it, please. :-)
User interface changes
No obvious changes.
API changes
No obvious changes.
Original report by [yched]
Currently, an EntityFieldQuery with a FieldCondition checks node_access grants, while an EFQ with only EntityConditions performs no check. That's inconsistent and confusing.
That's because field_sql_storage_field_storage_query() hardcodes an 'entity_field_access' tag on the query, which triggers _node_query_node_access_alter().
Side issue: the impact on the query is then hardcoded in _node_query_node_access_alter(), no matter what the field storage engine (currently, messes with the infamous {field_data_table}.etid column, which makes this etid construct de-facto compulsory for any other storage backend that want to store fields in SQL).
Discussing this with Damz, we agreed that :
- node_access check should be supported but not *hardcoded* in EFQ. It's up to the caller to add the tag himself.
--> The tag should be removed from field_sql_storage_field_storage_query().
--> Support for the tag should be added for EFQs without field conditions
- Ideally, the field storage engine should also handle dealing with the 'entity_field_access' tag if it was set.
That's probably for a separate issue, though ?
Comment | File | Size | Author |
---|---|---|---|
#11 | 997394.testonly.patch | 3.93 KB | BTMash |
Comments
Comment #1
catchSubscribing.
Comment #2
yched CreditAttribution: yched commented#1302228: field_has_data() returns inconsistent results – possible data loss was marked as a duplicate.
However, this latter issue outlines a nasty consequence : field_has_data() relies on EFQ and thus gets affected by node_access rules. It can then incorrectly return "nope, the field has no data".
This badly affects field_update_field() and the Field UI, which use field_has_data() to only allow some updates if the field is empty.
Comment #3
Damien Tournoud CreditAttribution: Damien Tournoud commentedAn easy fix for those queries is to add an
'account'
in the metadata:Comment #4
catchThe other issue was critical, bumping this one as well, account metadata sounds like a good quick fix.
Comment #5
yched CreditAttribution: yched commentedThe EFQ behavior described in the OP (sometimes node_access, sometimes not) sounds broken to me, but I'm not sure we can change that in D7 now (i.e. removing hardcoded node_access on some existing EFQs) ?
In that case, #3 might be our only workaround for D7. But for D8 ? Do we really want to leave EFQ that way ?
Comment #6
smk-ka CreditAttribution: smk-ka commentedSubscribing.
Comment #7
yched CreditAttribution: yched commentedDamien's workaround in #3 works as a hotfix for #1302228: field_has_data() returns inconsistent results – possible data loss, so I reopened that issue and posted a patch over there.
Leaving this issue open for the more general - IMO broken - behavior of EFQ regarding node_access.
Comment #8
chx CreditAttribution: chx commentedThis must be an oversight in the entity.inc code -- there we could add a check whether we are on node or not. fields are trickier because the entity type can change result row by result row.
Comment #9
tim.plunkettTagging.
Comment #10
aspilicious CreditAttribution: aspilicious commentedsummary is written
Comment #11
BTMash CreditAttribution: BTMash commentedAs mentioned above, currently, writing out entity field queries will return empty resultsets when dealing with field conditions. As far as I have see, the the specific scenario where this happens is when:
I'm attaching a test that shows how this fails in D8 but if you want to replicate it in D7, here is one (somewhat lengthy) example process.
By this lengthy point, you should end up with being able to see a resultset as an admin user (or a user that has the ability to bypass node access rules / view all nodes) and being able to see no results as an anonymous user.
The cause of this stems from a check in
hook_field_storage_query()
where it adds the tagentity_field_access
. The node module implements this tag for query alters and thus a check against non-node entities ends up here whereby the query gets joined with node_access tables.On a side note, since this was mentioned in #5 and #7, there seems to be a generic issue on this in #777578: Add an entity query access API and deprecate hook_query_ENTITY_TYPE_access_alter() though that issue also seems to be somewhat stuck.
Comment #13
BTMash CreditAttribution: BTMash commentedThis issue seems to be a duplicate of #1571104: Can't access non-node entities with EntityFieldQuery so I'm marking this as a duplicate as that issue seems to have more activity.
Comment #14
BerdirIt might be a duplicate of what you're reporting in #11, but I'm not sure the issues actual purpose is a duplicate. As per @yched's comment in #7:
Maybe re-open as a non-critical issue (not sure if bug or task)? I'm quite confused by the current behavior, as stated before.
Comment #14.0
BerdirAddition of Issue Summary at DrupalCon Denver Core sprint
Comment #15
m4oliveiFor those of us led to this issue and looking for a practical solution, would it suffice to simply always add
addTag('node_access')
to an EFQ (if it's a node query)? That way you don't have to remember that it's OK to leave it off if you do have field conditions.Thanks a bunch!