- could link to query conditions for DB API, as it's broadly the same system. For instance, there is no explanation of what operators and values are possible here.
- details in comments could do with being folded in

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jhodgdon’s picture

Issue tags: +Needs backport to D7

A patch would be a great way to explain the changes you are suggesting.

joachim’s picture

Hmm maybe the problem is that I was looking at the docs for fieldCondition() rather than addFieldCondition(), which has rather more: http://api.drupal.org/api/drupal/includes!entity.inc/function/EntityFiel....

And now I'm not sure which I should use. Judging by the source of fieldCondition(), I should be using that one, since it appears to be a wrapper (and therefore a helper?) to addFieldCondition(). Which suggests the more detailed docs are in the wrong place.

The deeper problem is that because for reasons I don't understand, the EFQ object is actually nothing to do with Database API but its own thing that partially mimics it, the excellent documentation for that (http://drupal.org/node/310086) doesn't fully apply.

joachim’s picture

Issue tags: +Novice

I think as a first step, the meat of what's on addFieldCondition() needs to be moved/copied to fieldCondition().

The former, as far as I can tell, is an internal method, and the latter is the one that's meant to be used. So that's the one that needs detail.

joachim’s picture

Also, both of these need to state which way round the non-symmetric comparison operators work. When you use '<', which is bigger than which?

ar-jan’s picture

Version: 8.0.x-dev » 7.x-dev
Issue summary: View changes
Issue tags: +Amsterdam 2014

This is not applicable to Drupal 8 anymore with the refactoring of EntityFieldQuery in #1801726: EntityFieldQuery v2.

Here's the change record: https://www.drupal.org/node/1827278.

The equivalent can now be found in core/lib/Drupal/Core/Entity/Query/QueryInterface.php.

There is an issue asking for docs cleanup of the D8 version here: #1827476: Docs cleanup for EFQ v2.

Setting version to 7.x.

ar-jan’s picture

Status: Active » Needs review
FileSize
2.37 KB

Here's a first patch that just copies the docs from addFieldCondition to fieldCondition.

I'm not sure how the comparison operators work and should be described.

ar-jan’s picture

#4 is still relevant in D8 though, I think it makes most sense to make that a separate issue that can be backported to D7.

oenie’s picture

Issue tags: -Amsterdam 2014 +Amsterdam2014

fixing the amsterdam sprint tag to amsterdam2014

oenie’s picture

Issue tags: -Amsterdam 2014 +Amsterdam2014

fixing the amsterdam sprint tag to amsterdam2014

jhodgdon’s picture

Version: 7.x-dev » 8.0.x-dev
Status: Needs review » Needs work

Due to #7, I think we should leave this as Drupal 8, fix the docs there, and then backport and add the other D7 stuff.

jp.stacey’s picture

Issue summary: View changes
Status: Needs work » Needs review
Issue tags: +SprintWeekend2016
Related issues: +#1827476: Docs cleanup for EFQ v2

The quoted issue #1827476: Docs cleanup for EFQ v2 is really only covering the exists() method right now. Nonetheless, it looks like the Drupal 8 docs for this method have been very much cleaned up.

I therefore think this is fixed in 8.0.x and now needs backporting, but marking as Needs Review so someone else can confirm.

jhodgdon’s picture

Version: 8.0.x-dev » 7.x-dev
Status: Needs review » Active

Agreed, the Drupal 8 docs appear to be in good shape now. So yes let's move this to Drupal 7.

felribeiro’s picture

Status: Active » Needs review
FileSize
2.27 KB

Status: Needs review » Needs work

The last submitted patch, 13: 1622964-13-D7.patch, failed testing.

jhodgdon’s picture

  1. +++ b/includes/entity.inc
    @@ -688,14 +688,36 @@
    +   *   The column that should hold the value to be matched, defined in the
    +   *   hook_field_schema() of this field. If this is omitted then the query
    +   *   will find only entities that have data in this field, using the
    +   *   entity and property conditions if there are any.
    

    This documentation is very confusing to me, as to what happens if $column is omitted... and I think we should fix it on addFieldCondition too (copied from there).

    So. Looking at the code and delving deep... I believe what it is trying to say is that if you omit $column, then all of the other parameters you pass to this function are ignored, except $field, and this call to addFieldCondition() or fieldCondition() will just be adding a condition that says that the field has a value, rather than testing the value itself.

    Can we reword these two @param docs so this is clearer?

  2. +++ b/includes/entity.inc
    @@ -688,14 +688,36 @@
        *   The operator to be used to test the given value.
    +   *   Possible values:
    

    The rest of the comments in this review are small "nitpick" things that need to be fixed.

    For this spot... This should be wrapped better. Documentation lines should go out to as close to 80 characters as possible without going over.

  3. +++ b/includes/entity.inc
    @@ -688,14 +688,36 @@
    +   *   two columns, 'color' and 'shape', and for entity id 1, there are two
    

    id should be ID

  4. +++ b/includes/entity.inc
    @@ -688,14 +688,36 @@
    +   *   corresponding to 'red circle', however if you pass 'red' and 'circle' as
    

    punctuation:

    ... circle'; however, if ...

Ankit Agrawal’s picture

Assigned: Unassigned » Ankit Agrawal
Issue tags: +drupalconasia2016
Ankit Agrawal’s picture

Assigned: Ankit Agrawal » Unassigned
felribeiro’s picture

Status: Needs work » Needs review
FileSize
4.4 KB
4.1 KB

Patch to fix the points in #15
Thanks @jhodgdon

jhodgdon’s picture

Status: Needs review » Needs work

Looking very good! Just a couple of very small things to fix:

  1. +++ b/includes/entity.inc	2016-03-11 14:34:01.420062332 -0300
    @@ -688,14 +688,36 @@
    +   *   two columns, 'color' and 'shape', and for entity ID 1, there are two ¶
    

    nitpick: There is an extra space at the end of this line.

    You should be able to set up your code editor to either remove or highlight end-of-line spaces.

  2. +++ b/includes/entity.inc	2016-03-11 14:34:01.420062332 -0300
    @@ -791,7 +815,7 @@
    -   *   two columns, 'color' and 'shape', and for entity id 1, there are two
    +   *   two columns, 'color' and 'shape', and for ID 1, there are two
    

    The word "entity" got taken out of this line by mistake I think?

Girish-jerk’s picture

Status: Needs work » Needs review
FileSize
4.48 KB

Hi jhodgdon,
I have recreated a patch for #19.
Thanks

Status: Needs review » Needs work

The last submitted patch, 20: 1622964-d7-20.patch, failed testing.

snehi’s picture

@Girish-jerk please update with interdiff.

snehi’s picture

Status: Needs work » Needs review
FileSize
4.54 KB
1.34 KB

New patch with interdiff.

Status: Needs review » Needs work

The last submitted patch, 23: 1622964-23.patch, failed testing.

jhodgdon’s picture

Status: Needs work » Reviewed & tested by the community

The test failure is unrelated to the patch, so ignore that. The patch looks good. Thanks!

Fabianx’s picture

Issue tags: +Pending Drupal 7 commit

Marked for commit, thanks!

stefan.r’s picture

Looks a lot better, thanks!

+++ b/includes/entity.inc
@@ -688,14 +688,36 @@ class EntityFieldQuery {
+   *   conditions, it will appear in the  results - by default queries will run

To be fixed on commit: double space, and s/-/-- (seems we don't do — in docs ;)

David_Rothstein’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: -Pending Drupal 7 commit

Committed to 7.x - thanks!

I fixed #27 on commit, but also fixed it in the original location (since this part of the comment was just copied from the original on addFieldCondition()):

diff --git a/includes/entity.inc b/includes/entity.inc
index ed6070f..e80ce3b 100644
--- a/includes/entity.inc
+++ b/includes/entity.inc
@@ -714,7 +714,7 @@ class EntityFieldQuery {
    *   two columns, 'color' and 'shape', and for entity ID 1, there are two
    *   values: red/square and blue/circle. Entity ID 1 does not have values
    *   corresponding to 'red circle'; however if you pass 'red' and 'circle' as
-   *   conditions, it will appear in the  results - by default queries will run
+   *   conditions, it will appear in the results -- by default queries will run
    *   against any combination of deltas. By passing the conditions with the
    *   same $delta_group it will ensure that only values attached to the same
    *   delta are matched, and entity 1 would then be excluded from the results.
@@ -818,7 +818,7 @@ class EntityFieldQuery {
    *   two columns, 'color' and 'shape', and for entity ID 1, there are two
    *   values: red/square and blue/circle. Entity ID 1 does not have values
    *   corresponding to 'red circle', however if you pass 'red' and 'circle' as
-   *   conditions, it will appear in the  results - by default queries will run
+   *   conditions, it will appear in the results -- by default queries will run
    *   against any combination of deltas. By passing the conditions with the
    *   same $delta_group it will ensure that only values attached to the same
    *   delta are matched, and entity 1 would then be excluded from the results.

  • David_Rothstein committed 3588007 on 7.x
    Issue #1622964 by felribeiro, snehi, ar-jan, Girish-jerk, jhodgdon,...

Status: Fixed » Closed (fixed)

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