Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Comment | File | Size | Author |
---|---|---|---|
#23 | interdiff.txt | 1.34 KB | snehi |
#23 | 1622964-23.patch | 4.54 KB | snehi |
#20 | 1622964-d7-20.patch | 4.48 KB | Girish-jerk |
#18 | interdiff-1622964-13-18.txt | 4.1 KB | felribeiro |
#18 | 1622964-18-D7.patch | 4.4 KB | felribeiro |
Comments
Comment #1
jhodgdonA patch would be a great way to explain the changes you are suggesting.
Comment #2
joachim CreditAttribution: joachim commentedHmm 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.
Comment #3
joachim CreditAttribution: joachim commentedI 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.
Comment #4
joachim CreditAttribution: joachim commentedAlso, both of these need to state which way round the non-symmetric comparison operators work. When you use '<', which is bigger than which?
Comment #5
ar-jan CreditAttribution: ar-jan commentedThis 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.
Comment #6
ar-jan CreditAttribution: ar-jan commentedHere'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.
Comment #7
ar-jan CreditAttribution: ar-jan commented#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.
Comment #8
oenie CreditAttribution: oenie commentedfixing the amsterdam sprint tag to amsterdam2014
Comment #9
oenie CreditAttribution: oenie commentedfixing the amsterdam sprint tag to amsterdam2014
Comment #10
jhodgdonDue to #7, I think we should leave this as Drupal 8, fix the docs there, and then backport and add the other D7 stuff.
Comment #11
jp.stacey CreditAttribution: jp.stacey at Magnetic Phield commentedThe 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.
Comment #12
jhodgdonAgreed, the Drupal 8 docs appear to be in good shape now. So yes let's move this to Drupal 7.
Comment #13
felribeiro CreditAttribution: felribeiro at CI&T commentedComment #15
jhodgdonThis 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?
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.
id should be ID
punctuation:
... circle'; however, if ...
Comment #16
Ankit Agrawal CreditAttribution: Ankit Agrawal as a volunteer commentedComment #17
Ankit Agrawal CreditAttribution: Ankit Agrawal as a volunteer commentedComment #18
felribeiro CreditAttribution: felribeiro at CI&T commentedPatch to fix the points in #15
Thanks @jhodgdon
Comment #19
jhodgdonLooking very good! Just a couple of very small things to fix:
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.
The word "entity" got taken out of this line by mistake I think?
Comment #20
Girish-jerk CreditAttribution: Girish-jerk at UniMity Solutions Pvt Limited commentedHi jhodgdon,
I have recreated a patch for #19.
Thanks
Comment #22
snehi CreditAttribution: snehi as a volunteer and at Publicis Sapient for Publicis Sapient commented@Girish-jerk please update with interdiff.
Comment #23
snehi CreditAttribution: snehi as a volunteer and at Publicis Sapient for Publicis Sapient commentedNew patch with interdiff.
Comment #25
jhodgdonThe test failure is unrelated to the patch, so ignore that. The patch looks good. Thanks!
Comment #26
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedMarked for commit, thanks!
Comment #27
stefan.r CreditAttribution: stefan.r commentedLooks a lot better, thanks!
To be fixed on commit: double space, and s/-/-- (seems we don't do — in docs ;)
Comment #28
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedCommitted 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()):