Posted by plach on July 1, 2011 at 10:41am
7 followers
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | field system |
| Category: | task |
| Priority: | critical |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
| Issue tags: | needs backport to D7 |
Issue Summary
At the moment there is no way to perform an entity field query with a specifc condition on field language or delta. This might be a really needed capability, see for instance #1164852-22: Inconsistencies in field language handling.
Comments
#1
Marking as critical task that needs to be backported to D7, since this is needed to properly fix the issue cited above, which needs to be done before D7.5 is released.
#2
The following columns are added by field_sql_storage: entity_type, bundle, delete, entity_id, revision_id, language, delta to every field storage table. Out of these, entity_type, bundle, entity_id, revision_id are part of the entity. But delete, language and delta could and should be query-able independently. A new method, quite likely as there is no way to squeeze these into the existing methods. fieldCondition won't work because nothing stops you to add a field column named 'delta' for example ( and it will work as field_sql_storage adds the table name to the field columns while the metadata is named just that).
There is nothing to stop us from adding yet another argument to fieldCondition but oh god please no. it's ugly already. Just have a new method. fieldMetaCondition? Add one for each? fieldDeleteCondition, fieldLanguageCondition, fieldDeltaCondition?
Note that this / these new method(s) likely need delta grouping with field conditions. And maybe language grouping too.
#3
Subscribe
#4
webchick suggest fieldValueCondition -- maybe? I can see the logic but I am wondering whether it will confuse people due to some basic fields like text using a value column.
#5
fieldValueCondition looks confusing to me, I'd be on board for fieldMetaCondition or fieldPropertyCondition
#6
Having fieldPropertyCondition next to propertyCondition would be awful.
+1 for fieldMetaCondition.
#7
Here is a first attempt, docs are yet to be written and so are tests. I'd need feedback before going on.
FWIW the patch seems to work on my early tests for the other issue.
#8
This one should be better. Only tests are missing.
#9
Definitely getting there: comments aplenty and using an anonymous callback to achieve clean code is especially welcome. Of course needs tests. But, the reason I CNW'd it because if you compare fieldCondition and fieldMetaCondition you will be very hard pressed to find any difference but the name of the property we are loading the very same data. Yes to having a separate fieldConditions and fieldMetaConditions array -- but why not have a protected method like
protected function foo($property, $field, $column, $value, $operator, $delta_group, $language_group) { copy of fieldCondition just change $this->fieldConditions to $this->property}and then make fieldCondition and fieldMetaCondition call it with the respective $property.#10
@chx: Thanks!
Implemented the suggestion from #9 and added tests. If there are no more pending concerns we should be ready to go.
Attached there is the 7.x version where the anonynous function has been replaced with a regular callback.
#11
#12
I don't think we should allow querying on 'deleted'. deleted is special, and is handled separately by the EFQ::deleted() method. Allowing it here as well would probably cause some weird clashes.
Other than that, looks good to me - and terrific tests :-)
#13
@yched: thanks for reviewing!
Are you suggesting to remove 'deleted' mentions from PHP docs and tests? Because besides those there is nothing specific about in the patch. Personally I'd be ok with that, I was a bit confused while working on tests when I learnt about the EFQ::deleted() method, but I'd like to hear also chx's feedback, since he originally proposed to support the 'deleted' column.
#14
Yes, I'd say we don't need to add code to explicitly prevent a condition on 'deleted', but we should remove it from the suggestions in the phpdoc, and probably add a note that the delete() should be used for conditions on 'deleted'.
#15
If we drop support for the 'deleted' column we might introduce two fixed methods as proposed in #2: fieldLanguageCondition and fieldDeltaCondition, removing the generic fieldMetaCondition.
#16
@plach : I don't think I have a strong opinion on this. chx maybe ?
#17
removing deleted is fine
#18
Spoke with @chx in irc and he approves the direction, let's see if the implementation is good.
Again in the 7.x version the anonynous function has been replaced with a regular callback.
#19
Supported by yched positive review I am confident in marking this terrific patch RTBC (provided the bot comes back green).
#20
I rerolled with even better comments.
#21
Reviewing this, I mainly had concerns around documentation... there was nothing in the docs that explained to me why 'meta' conditions were different from regular conditions, and there was still a reference to + * @see EntityFieldQuery::fieldMetaCondition() which no longer existed.
chx addressed those in #20, and so I committed and pushed that to 8.x and 7.x.
...which I just noticed was the wrong thing to do. :( So chx is going to roll a new interdiff patch. Sorry. :(
#22
Here's a followup -- it's the interdiff between the D7 and the D8 patches.
#23
#24
here it's again hoping the bot picks it up as 7.x
#25
Awesome, thanks. Committed/pushed to 7.x.
#26
Automatically closed -- issue fixed for 2 weeks with no activity.