Missing summary/param/return docs for several QueryInterface methods

Hi Joachim,

Could you please expand a little further on:

• Missing param: I´m guessing that you mean that parameters should have a description. Am I correct?
• Missing return: what do you mean by this?

Thanks in advance,

Just giving a try.

@er.pushpinderrana on your patch the description is still missing and you need a blank line before the return.

Edit: I accidentally deleted your file because when I did the request I got an error and I thought it was regarding my file in the uploaded files list.

So sorry about that.

Is there a way to rollback this action?

With some additions and modifications.

Here is a review of the latest patch:

1. +++ b/core/lib/Drupal/Core/Entity/Query/QueryInterface.php
   @@ -111,19 +111,28 @@ public function notExists($field, $langcode = NULL);
   +   * Queries for defining range.
   

   Please take a look at

   https://www.drupal.org/node/1354#functions

   The first line descriptions in this patch do not conform to our standards. The previous patch was better in this regard.

2. +++ b/core/lib/Drupal/Core/Entity/Query/QueryInterface.php
   @@ -111,19 +111,28 @@ public function notExists($field, $langcode = NULL);
       * @param null $start
   +   *  The offset of the first row to return.
   

   Needs another space of indentation.

   Also we should fix the type. It should be int|null.

   Also the first patch on this issue had:

   +   *   The first record from the result set to return. If NULL, removes any
   +   *   range directives that are set.
   

   That seems like very useful information about what NULL means. We should make sure this gets into the patch.

3. +++ b/core/lib/Drupal/Core/Entity/Query/QueryInterface.php
   @@ -111,19 +111,28 @@ public function notExists($field, $langcode = NULL);
       * @param null $length
   +   *  The maximum number of rows to return.
   

   See previous note.

4. +++ b/core/lib/Drupal/Core/Entity/Query/QueryInterface.php
   @@ -111,19 +111,28 @@ public function notExists($field, $langcode = NULL);
       * @return \Drupal\Core\Entity\Query\QueryInterface
       *   The called object.
   

   This should be changed to say:

   @return $this

5. +++ b/core/lib/Drupal/Core/Entity/Query/QueryInterface.php
   @@ -111,19 +111,28 @@ public function notExists($field, $langcode = NULL);
       * @return \Drupal\Core\Entity\Query\QueryInterface
       *   The called object.
   

   Should be @return $this

Modified the patch as per suggestion.

@jhodgdon
How are you? Its been long time . By the by I am in new Orleans, If you are around would like to meet you.

I have applied the comments #6 you are mentioned.

+++ b/core/lib/Drupal/Core/Entity/Query/QueryInterface.php
@@ -134,9 +134,12 @@ public function notExists($field, $langcode = NULL);
+   *   The maximum number of rows to return. If there is no rows returns NULL,
+   * @return $this

"If there *are*..."

Also, there should be a blank line between last @param and @return.

+++ b/core/lib/Drupal/Core/Entity/Query/QueryInterface.php
@@ -134,9 +134,12 @@ public function notExists($field, $langcode = NULL);
+   * @param int|null $length
+   *   The maximum number of rows to return. If there is no rows returns NULL,

A couple of points for clarification:

• Is passing a $length of null different from passing 0; or should it be?
• What returns NULL if there are no rows?

I'm setting this back to needs work for this and #11.

@John did not get you.
What you want to place here int|null|???

Done as mentioned in 11

I'll try to clarify a bit.

Is passing a $length of null different from passing 0; or should it be?
Does setting $length to NULL have any special meaning? eg passing NULL as $start has the special meaning of removing the range filter.

Possible options could be (there's probably others):

• Passing NULL returns all results.
• Passing NULL is the same as passing 0.
• Passing NULL sets the length as "undefined" and specified by the implementation.

What returns NULL if there are no rows?
If it is this function that return NULL, "If there is no rows returns NULL" should be moved to the @return section. I don't think this is the case as it would break how QueryInterface is used.

If it describes the end result then it should be moved to before the @param section as it describes what happens at the execute() stage. Again, I don't think this is the case either as execute() has been defined as @return int|array so should never return NULL.

The third option is to remove it as it does not describe anything about the function.

I hope this helps.

I have just noticed that there should be a short description of what the function does at the top of the docblock, so it should start like:

/**
 * Enables a range for the query.
 *
 * @param int|null $start
 *   The first record from the result set to return. If NULL, removes any
...
 */

Added short description as per the suggestion.

Thanks for the patches! It is looking better but still needs some work:

1. +++ b/core/lib/Drupal/Core/Entity/Query/QueryInterface.php
   @@ -134,9 +134,15 @@ public function notExists($field, $langcode = NULL);
   +   * Enables a range for the query.
   

   Can we make a better first line description for this? I think I don't like the word "enables" much. Maybe "Adds a range to" or "Defines the range of" or something like that?

2. +++ b/core/lib/Drupal/Core/Entity/Query/QueryInterface.php
   @@ -134,9 +134,15 @@ public function notExists($field, $langcode = NULL);
   +   *   The maximum number of rows to return. If there are no rows returns NULL.
   

   This doesn't make sense to me. This function doesn't return NULL, it returns the query object. Besides, if it did return NULL, we would document that on the @return, not on the @param. So... what we need to document here is what passing in NULL for $length does to the query.

3. +++ b/core/lib/Drupal/Core/Entity/Query/QueryInterface.php
   @@ -134,9 +134,15 @@ public function notExists($field, $langcode = NULL);
   +   * @return $this
       *   The called object.
   

   When you do @return $this, do not provide a description like "The called object" -- just take that line out.

4. +++ b/core/lib/Drupal/Core/Entity/Query/QueryInterface.php
   @@ -147,7 +153,7 @@ public function range($start = NULL, $length = NULL);
   +   * @return $this
       *   The called object.
   

   See above note

5. +++ b/core/lib/Drupal/Core/Entity/Query/QueryInterface.php
   @@ -157,7 +163,7 @@ public function sort($field, $direction = 'ASC', $langcode = NULL);
   +   * @return $this
       *   The called object.
   

   See above note. There are several more of these in the patch. Please fix them all.

I have added change as per suggestion.

Below are the changes added for each comment:
1. Added description as "Defines the range of query."

2. I have not changed anything for this suggestion.

3. Removed the line "The called object."

4. Removed the line "The called object."

5. Removed the line "The called object."

I have also removed the line "The called object." from two funtions.

public function count();

public function tableSort(&$headers);

Not quite...

1. +++ b/core/lib/Drupal/Core/Entity/Query/QueryInterface.php
   @@ -134,10 +134,15 @@ public function notExists($field, $langcode = NULL);
   +   * Defines the range of query.
   

   needs "the" before query.

2. +++ b/core/lib/Drupal/Core/Entity/Query/QueryInterface.php
   @@ -134,10 +134,15 @@ public function notExists($field, $langcode = NULL);
   +   *   The maximum number of rows to return. If there are no rows returns NULL.
   

   Again, this is a *parameter*. Saying "If there are no rows returns NULL" doesn't make sense here.

   What needs to be documented is what happens if you pass in NULL for $length.

3. +++ b/core/lib/Drupal/Core/Entity/Query/QueryInterface.php
   @@ -147,8 +152,7 @@ public function range($start = NULL, $length = NULL);
       *   Language code (optional).
   -   * @return \Drupal\Core\Entity\Query\QueryInterface
   -   *   The called object.
   +   * @return $this
   

   Please also add a blank line before @return $this.

   Well. Actually, these changes are out of scope for this issue, and should be removed. This issue is only about the range() function. Sorry for suggesting this fix in my earlier review. Let's just fix up range().

I have added suggested changes.

Reverted all the changes of other functions except range() function.

+++ b/core/lib/Drupal/Core/Entity/Query/QueryInterface.php
@@ -134,10 +134,15 @@ public function notExists($field, $langcode = NULL);
+   * @param int|null $length
+   *   The maximum number of rows to return.

Hm, I'll try to say this again (for the third time).

In the @param line, it says int|null.

So, we need to document what it means if you pass in an int (this is done), and what it means if you pass in a NULL (this is not done).

So... Also I looked at QueryInterface, and there are two more methods with no documentation (similar to range()). These are:
- sort()
- accessCheck

There are also several other methods that are lacking documentation for either param or return:
- andConditionGroup
- orConditionGroup
- notExists (the @return should be @return $this)
- exists (the @return should be @return $this)
- condition (the @return should be @return $this)
- getEntityTypeId

I'm wondering if we should expand the scope of this issue to fix all of these problems, or file separate issues? Hm.... Given https://www.drupal.org/core/scope I think it would be appropriate to expand the scope to this entire file. So, updating title and issue summary.

In the @param line, it says int|null.

So, we need to document what it means if you pass in an int (this is done), and what it means if you pass in a NULL (this is not done).

1. If $start and $length are NULL, then complete result is return.
2. If $start is not NULL and $length is NULL, then empty result set will be return.

I have given a try. Please review and share your thoughts.

Much better, thanks! A few things to fix:

1. +++ b/core/lib/Drupal/Core/Entity/Query/QueryInterface.php
   @@ -90,7 +90,7 @@ public function getEntityTypeId();
   -   * @return \Drupal\Core\Entity\Query\QueryInterface
   +   * @return $this
       * @see \Drupal\Core\Entity\Query\andConditionGroup
   

   Between @return and @see, we need a blank line.

2. +++ b/core/lib/Drupal/Core/Entity/Query/QueryInterface.php
   @@ -134,19 +136,31 @@ public function notExists($field, $langcode = NULL);
   +   *   The maximum number of rows to return. If $start and $length are NULL,
   +   *   then complete result is return. If $start is not NULL and $length is
   +   *   NULL, then empty result set will be return.
   

   This is much clearer, thanks!

   Some minor grammatical fixes -- better wording would be:

   ... then a complete result set will be generated.

   ... then an empty result set will be generated.

   Note: Since the return value of this method is the class itself, let's not use the phrase "will be returned" here; instead, "will be generated".

3. +++ b/core/lib/Drupal/Core/Entity/Query/QueryInterface.php
   @@ -134,19 +136,31 @@ public function notExists($field, $langcode = NULL);
       * @param $field
   -   *   Name of a field.
   +   *   Name of a field on which to sort.
       * @param string $direction
   +   *   The direction to sort. Legal values are "ASC" and "DESC".
   +   *   Any other value will be converted to "ASC".
   

   Nice, thanks! This is good documentation. Very clear.

4. +++ b/core/lib/Drupal/Core/Entity/Query/QueryInterface.php
   @@ -134,19 +136,31 @@ public function notExists($field, $langcode = NULL);
       * @return \Drupal\Core\Entity\Query\QueryInterface
       *   The called object.
   

   Should say

   @return $this

5. I listed 8 methods that needed attention in comment #25. I don't think that they are all covered in this patch. I only count 5 being updated. Please fix the others too.

Hi jhodgdon,

Thank you so much for support.

I have worked on the mentioned mentioned above. Please review.

I have worked on following things.

Point 1, 2, 4 Done.

Point 5 :
1. Added data type names for param in condition(), exists(), notExists(), sort()
2. Added documentation line to accessCheck(). Remaining methods mentioned in issue summary exist documentation.

Thanks

Looking better, thanks! Still a couple of things to fix:

1. +++ b/core/lib/Drupal/Core/Entity/Query/QueryInterface.php
   @@ -128,25 +131,36 @@ public function notExists($field, $langcode = NULL);
       * @return \Drupal\Core\Entity\Query\QueryInterface
       *   The called object.
   

   Should be @return $this

2. +++ b/core/lib/Drupal/Core/Entity/Query/QueryInterface.php
   @@ -177,8 +191,12 @@ public function count();
   +   * Enable access check for this query.
   

   It seems, given the @param description below, that this should be:

   Enables or disables access checking for this query.

   [note: should be Enables not Enable for the first-line description]

3. On the method andConditionGroup(), it has a @return line with a type, but not any documentation:
   

      * @return \Drupal\Core\Entity\Query\ConditionInterface
      */
     public function andConditionGroup();
   

   All @return lines [except @return $this or @return static] should have a documentation line below them. This is also the problem for orConditionGroup() and getEntityTypeId().

I have updated the patch as per your suggestions.

Not quite there yet:

1. +++ b/core/lib/Drupal/Core/Entity/Query/QueryInterface.php
   @@ -128,25 +131,36 @@ public function notExists($field, $langcode = NULL);
       * @return \Drupal\Core\Entity\Query\QueryInterface
       *   The called object.
   

   Any time you see "the called object", it should be replaced by

   @return $this

2. +++ b/core/lib/Drupal/Core/Entity/Query/QueryInterface.php
   @@ -212,6 +230,7 @@ public function execute();
       * @return \Drupal\Core\Entity\Query\ConditionInterface
   +   *   The called object.
       */
      public function andConditionGroup();
   

   No, this does not return the called object. It returns a ConditionInterface, which is something else.

3. +++ b/core/lib/Drupal/Core/Entity/Query/QueryInterface.php
   @@ -240,6 +259,7 @@ public function andConditionGroup();
       * @return \Drupal\Core\Entity\Query\ConditionInterface
   +   *   The called object.
       */
      public function orConditionGroup();
   

   Same here.

4. I still do not see a fix for the getEntityTypeId() method, which needs to have a documentation line added to its @return section.

Done changes as per said.

Thank!!

Almost there! One more suggestion on the new documentation added in this latest patch:

+++ b/core/lib/Drupal/Core/Entity/Query/QueryInterface.php
@@ -212,8 +228,9 @@ public function execute();
+   *   An object holding AND group of conditions.

Can we reword this (and the similar OR documentation line)... it seems kind of awkward and a bit misleading. Something like:

A condition object whose conditions will be combined with AND.

Changing as per suggestion in #38.

Hooray for the QueryInterface having actual documentation! Thanks!

Thank you for your work on cleaning up Drupal core's code style!

In order to fix core coding standards in a maintainable way, all our coding standards issues should be done on a per-rule basis across all of core, rather than fixing standards in individual modules or files. We should also separate fixes where we need to write new documentation from fixes where we need to correct existing standards. This all should be done as part of #2571965: [meta] Fix PHP coding standards in core, stage 1. A good place to start is the child issues of #2572645: [Meta] Fix 'Drupal.Commenting.FunctionComment' coding standard.

For background information on why we usually will not commit coding standards fixes that aren't scoped in that way, see the core issue scope guidelines, especially the note about coding standards cleanups. That document also includes numerous suggestions for scoping issues including documentation coding standards cleanups.

Contributing to the overall plan above will help ensure that your fixes for core's coding standards remain in core the long term.

In the case of this issue, these are the lines of new documentation:

+++ b/core/lib/Drupal/Core/Entity/Query/QueryInterface.php
@@ -18,6 +18,7 @@
+   *   The entity type ID.

@@ -128,27 +132,37 @@ public function notExists($field, $langcode = NULL);
+   * Defines the range of the query.
...
+   *   The first record from the result set to return. If NULL, removes any
+   *   range directives that are set.
...
+   *   The maximum number of rows to return. If $start and $length are NULL,
+   *   then a complete result set will be generated. If $start is not NULL and
+   *   $length is NULL, then an empty result set will be generated.
...
+   * Sorts the result set by a given field.
...
+   *   The direction to sort. Legal values are "ASC" and "DESC".
+   *   Any other value will be converted to "ASC".

@@ -171,14 +184,17 @@ public function count();
+   * Enables or disables access checking for this query.
+   *
+   * @param bool $access_check
+   *   Whether access check is requested or not. Defaults to TRUE.

@@ -212,8 +228,9 @@ public function execute();
+   *   A condition object whose conditions will be combined with AND.

@@ -240,6 +257,7 @@ public function andConditionGroup();
+   *   A condition object whose conditions will be combined with OR.

So, the attached patch adds only those changes, plus two additional changes:

1. When a parameter is optional (providing a default value), our standards specify adding (optional) at the beginning of the textual description. https://www.drupal.org/node/1354#param
2. The typehint of null only for range() is so confusing and strange that I included correcting that as well; otherwise the descriptions for those parameters don't even make sense.

The rest of the fixes for adding and correcting data types (as well as the fix to indentation) can happen as part of the respective child issues of the meta.

I think I understand the new scope of this issue.

I'll review it.

1. I looked into
   

   +   *   The direction to sort. Legal values are "ASC" and "DESC".
   +   *   Any other value will be converted to "ASC".
   

   We dont usually use "legal" values

   ag "^[ ]*\*.*value.*[\"]" core | grep -i legal
core/lib/Drupal/Core/Database/Query/SelectInterface.php:404: * The direction to sort. Legal values are "ASC" and "DESC". Any other value
   or
   ag "^ *\* *.*[Ll]egal value" core

   core/lib/Drupal/Core/Database/Connection.php
   208:   *   returned. Legal values include PDO::FETCH_ASSOC, PDO::FETCH_BOTH,
   218:   *   unpredictable results or fatal errors. Legal values include:
   
   core/lib/Drupal/Core/Database/Query/SelectInterface.php
   404:   *   The direction to sort. Legal values are "ASC" and "DESC". Any other value

   "allowed" is more the norm in core

   ag "^[ ]*\*.*value.*[\"]" core | grep -i allowed
   or
   ag "^ *\* *.*[Aa]llowed value" core

   So I'm going to change it to allowed values.

   There are two sentences that can be line wrapped. Going to do that.

   https://www.drupal.org/node/1354#drupal
   "Lines containing comments (including docblocks) must wrap as close to 80 characters as possible without going over, with a few exceptions (noted in the Tag Reference below)."
   -------------

   I also looked to see if " or ' was more common for quotes to use in comments to note allowed values and found both... so I dont see a clear decisions for quotes. Leaving those at ".

2. -   * @param null $start
   -   * @param null $length
   +   * Defines the range of the query.
   +   *
   +   * @param int|null $start
   +   *   (optional) The first record from the result set to return. If NULL,
   +   *   removes any range directives that are set.
   +   * @param int|null $length
   +   *   (optional) The maximum number of rows to return. If $start and $length
   +   *   are NULL, then a complete result set will be generated. If $start is
   +   *   not NULL and $length is NULL, then an empty result set will be
   +   *   generated.
   +   *

   I also looked at this section. Cause it seemed weird to talk about both start and length under only length... but it doesn't make sense before @param and it is against coding standards to put it between @param and @return... and $start was talked about first... so I think this is ok.

3. I looked at all the other changes (in context) and they look fine. I also used coder and the changes didn't introduce any violations. And I ran:
   phpcs -s -p --standard=./core/phpcs.xml.dist --extensions='php,module,inc,install,test,profile,theme,js,css,info,txt' core/lib/Drupal/Core/Entity/Query/QueryInterface.php
   which runs just the standards that are checked in core, and there are no violations.

------

So the interdiff is just a change from Legal to allowed, and wrapping a line. The rest of the changes are good. rtbc.

forgot to change status to rtbc.

A follow up issue to fix QueryInterface::sort()
so that it really only allows the two values and throws an error if another value for the direction arg is given.

The way it is now is brittle.
Mis-spell DESC as DSC or something and it will sort ascending.

+++ b/core/lib/Drupal/Core/Entity/Query/QueryInterface.php
@@ -134,17 +135,30 @@ public function notExists($field, $langcode = NULL);
+   *   The direction to sort. Allowed values are "ASC" and "DESC". Any other
+   *   value will be converted to "ASC".

This is a little complex. It looks like different implementations do different things.
\Drupal\Core\Database\Driver\pgsql\Select::orderBy() does

$direction = strtoupper($direction) == 'DESC' ? 'DESC NULLS LAST' : 'ASC NULLS FIRST';

\Drupal\Core\Database\Query\Select::orderBy does

$direction = strtoupper($direction) == 'DESC' ? 'DESC' : 'ASC';

\Drupal\Core\Config\Entity\Query\Query::execute() does

$direction = $sort['direction'] == 'ASC' ? -1 : 1;

\Drupal\Core\Entity\KeyValueStore\Query\Query::execute() does

$direction = $sort['direction'] == 'ASC' ? -1 : 1;

Tricky...

adding dev days tag, and also taking off novice, since this is currently tricky to know what the next step is.

some ideas for way forward
take out the "fix" to the docs for the sort order and make a separate issue to... make the implementations consistent? (but that would be a BC break and need to go into 9.x https://www.drupal.org/core/d8-allowed-changes)
or change the docs to say "oops. our different implementations do different things. go look at their docs."?
or maybe the implementations dont document.. themselves

side note:
$direction = $sort['direction'] == 'ASC' ? -1 : 1;
could really benefit from a const

So first I guess we need to
verify what the actual behavior of those implementations is when neither allowed value is given (what they actually default to)
and
check the docs on the implementations for their own accuracy

Marking 'needs work' for #45 and #46

1. I have updated the other missing docs.

2. I created a follow-up #3079258: Make SQL direction default behavior more consistent when an unknown direction is passed for #45.

Changes looking good,

Feedback, especially updating documentation:

1. +++ b/core/lib/Drupal/Core/Entity/Query/QueryInterface.php
   @@ -73,11 +74,11 @@ public function getEntityTypeId();
   +   * @param mixed|null $value
   

   I wonder if mixed should be converted to the scalars, since NULL is defined. Not sure about this.

2. +++ b/core/lib/Drupal/Core/Entity/Query/QueryInterface.php
   @@ -73,11 +74,11 @@ public function getEntityTypeId();
   +   * @param string|null $operator
   

   We should document what NULL does.

3. +++ b/core/lib/Drupal/Core/Entity/Query/QueryInterface.php
   @@ -86,7 +87,7 @@ public function getEntityTypeId();
   +   * @param string|null $langcode
   

   Docs should be updated, with NULL behaviour last.

4. +++ b/core/lib/Drupal/Core/Entity/Query/QueryInterface.php
   @@ -103,32 +104,36 @@ public function condition($field, $value = NULL, $operator = NULL, $langcode = N
       *   Language code (optional).
   

   We should document what NULL does.

5. +++ b/core/lib/Drupal/Core/Entity/Query/QueryInterface.php
   @@ -103,32 +104,36 @@ public function condition($field, $value = NULL, $operator = NULL, $langcode = N
       *   Language code (optional).
   

   NULL should be documented

6. +++ b/core/lib/Drupal/Core/Entity/Query/QueryInterface.php
   @@ -103,32 +104,36 @@ public function condition($field, $value = NULL, $operator = NULL, $langcode = N
   +   * @param int $limit
   

   Documentation indicates values other than int are valid.

7. +++ b/core/lib/Drupal/Core/Entity/Query/QueryInterface.php
   @@ -103,32 +104,36 @@ public function condition($field, $value = NULL, $operator = NULL, $langcode = N
       *   If not provided, one is automatically calculated.
   

   Mechanisms of automatic calculation should be summarised. or @see'd

8.    * @return $this
      * @see \Drupal\Core\Entity\Query\andConditionGroup
      * @see \Drupal\Core\Entity\Query\orConditionGroup
   

   While we're in the neighborhood, CS requires a blank line between @return and @see

The patch tries to address most points from #56(except point 7). It's still far from ideal but contains some progress.

Most of the points from comment #56 except for point 7 (repeated here for clarity):

+++ b/core/lib/Drupal/Core/Entity/Query/QueryInterface.php
@@ -103,32 +104,36 @@ public function condition($field, $value = NULL, $operator = NULL, $langcode = N
    *   If not provided, one is automatically calculated.

Mechanisms of automatic calculation should be summarised. or @see'd

Because this still needs to be documented I'm setting to needs work.

Here is an attempt to address point 7 from #56.

@init90, I am not the native speaker, but I would change it text to:
"If not provided, one is automatically calculated by incrementing the every next pager element value."
Otherwise, looks good to me.

Thanks, @alex-liannoy. I've updated the patch according to your suggestion.

The given $value can also be a boolean or integer.
E.g.:
$query->condition('status', NodeInterface::PUBLISHED);
where NodeInterface::PUBLISHED has an integer value of 1.

Actually the $value field can also be of an Drupal\Core\Entity\Query\ConditionInterface instance.
I guess it's better to mark the field as mixed then.

Updated patch with an improved comment.

rerolling

Patch #67 changes are visible on 9.1

+++ b/core/lib/Drupal/Core/Entity/Query/QueryInterface.php
@@ -33,8 +34,9 @@ public function getEntityTypeId();
+   * @param mixed $field
+   *   Name of the field being queried or an instance of ConditionInterface.

I think we can say string|\Drupal\Core\Condition\ConditionInterface here.

Rerolled patch #67 for 9.2.x, Made the change suggested in #70. Added the interdiff to reflect the same. Please review, thanks.

Seems like the change was missed in the last patch.

FILE: ...ar/www/html/core/lib/Drupal/Core/Entity/Query/QueryInterface.php
----------------------------------------------------------------------
FOUND 3 ERRORS AFFECTING 3 LINES
----------------------------------------------------------------------
  37 | ERROR | [ ] Missing parameter name
 250 | ERROR | [x] Return comment indentation must be 3 spaces, found
     |       |     1 spaces
 279 | ERROR | [x] Return comment indentation must be 3 spaces, found
     |       |     1 spaces

Updated as mentioned in #74. Please review.

Thanks! A few minor points:

1. +++ b/core/lib/Drupal/Core/Entity/Query/QueryInterface.php
   @@ -73,41 +75,48 @@ public function getEntityTypeId();
   +   *   Default to NULL, for most operators(except: 'IS NULL', 'IS NOT NULL') it
   

   Needs a space before ( i.e. "operators ("

2. +++ b/core/lib/Drupal/Core/Entity/Query/QueryInterface.php
   @@ -73,41 +75,48 @@ public function getEntityTypeId();
       * @see \Drupal\Core\Entity\Query\andConditionGroup
   -   * @see \Drupal\Core\Entity\Query\orConditionGroup
   +   * @see \Drupal\Core\Entity\Query\
   +   * @see \Drupal\Core\Entity\Query\ConditionInterface
   

   This part of the reroll looks incorrect, I think we should just keep the ConditionInterface line.

3. +++ b/core/lib/Drupal/Core/Entity/Query/QueryInterface.php
   @@ -246,7 +275,8 @@ public function andConditionGroup();
   -   * @return \Drupal\Core\Entity\Query\ConditionInterface
   +   * @return \Drupal\Core\Entity\Query\
   

   Again this looks wrong, the ConditionInterface reference was correct.

Addressing #76, Please have a look and advise.

Applied patch #77.The problems mentioned in #76 are rectified in this patch.

+++ b/core/lib/Drupal/Core/Entity/Query/QueryInterface.php
@@ -73,41 +75,48 @@ public function getEntityTypeId();
    * @see \Drupal\Core\Entity\Query\andConditionGroup
    * @see \Drupal\Core\Entity\Query\orConditionGroup

I still think these two are wrong - these classes don't exist.

Seems like andConditionGroup and orConditionGroup never existed as separate classes and reference to them was incidentally added in #1801726: EntityFieldQuery v2. I have changed that references to interface methods with the same names.

Thanks, this is looking good now.

This is looking really good, appreciate the effort folks have gone to here, especially those for whom English is their non native language, it's hard enough getting this right in your native language let alone a second one 🧠💪

1. +++ b/core/lib/Drupal/Core/Entity/Query/QueryInterface.php
   @@ -73,41 +75,48 @@ public function getEntityTypeId();
   +   *   Default to NULL, for most operators (except: 'IS NULL', 'IS NOT NULL') it
   

   I think this should be 'Defaults to NULL' not 'Default to NULL'

2. +++ b/core/lib/Drupal/Core/Entity/Query/QueryInterface.php
   @@ -73,41 +75,48 @@ public function getEntityTypeId();
   +   *   - 'IS NULL', 'IS NOT NULL': These operators ignore $value, so best way
   +   *     not specify value and use NULL.
   

   I'm not sure what we're trying to say here. Perhaps this could read 'These operators ignore $value, for that reason it is recommended to use a $value of NULL for clarity' ?

   We should also point people to exists and notExistsmethods via @see as they are preferred to IS NULL and IS NOT NULL when it comes to entity queries. (And in my experience those methods work first time, whereas I've always had trouble getting the right combination for IS NULL and IS NOT NULL)

3. +++ b/core/lib/Drupal/Core/Entity/Query/QueryInterface.php
   @@ -128,34 +138,50 @@ public function notExists($field, $langcode = NULL);
   +   *   If not provided, one is automatically calculated by incrementing the
   +   *   every next pager element value.
   

   I'm not sure what this means, suggest removing the word 'every' perhaps?

4. +++ b/core/lib/Drupal/Core/Entity/Query/QueryInterface.php
   @@ -128,34 +138,50 @@ public function notExists($field, $langcode = NULL);
   +   *   Any other value will be converted.
   

   I'm not sure what 'will be converted' means, can we elaborate on what it will be converted to or how it will be converted?

Thanks again for working on this

Updated the patch in #80 addressing #82.1, #82.2, #82.3 and #82.4. Kindly review.

To answer #82.4, ideally it should be `Any other value will be converted to "ASC"`, but I don't see that happening in any of the classes implementing QueryInterface. An issue has already been created to address this #3079258: Make SQL direction default behavior more consistent when an unknown direction is passed, where the code along with the doc comment for /core/lib/Drupal/Core/Entity/Query/QueryInterface::sort() needs to be updated.

Thank you!

Thanks for updating this, one more nit and then hopefully we're done:

+++ b/core/lib/Drupal/Core/Entity/Query/QueryInterface.php
@@ -73,41 +75,50 @@ public function getEntityTypeId();
+   *   Defaults to NULL, for most operators (except: 'IS NULL', 'IS NOT NULL') it
+   *   always makes the condition false.

This is now over 80 characters and needs rewrapping.

Updated patch in #83 addressing #84. Thanks!

Thanks! All review points have been addressed. I agree that solving the ASC/DESC issue can be deferred to that other issue and that the comment here is good enough for now.

Committed 9c9c981 and pushed to 9.2.x. Thanks!