Improve comments in \Drupal\Core\Entity\Query\Sql\Tables

Patch applies cleanly to 8.0.x and corrects the 3 issues above. Great stuff.

However I see there is an outstanding issue with:

    * @param $field_name
    *   Name of the field.
+   *
    * @return string
+   *
    * @throws \Drupal\Core\Entity\Query\QueryException
    */
   protected function ensureFieldTable($index_prefix, &$field, $type, $langcode, $base_table, $entity_id_field, $field_id_field) {

$field_name isn't a parameter for this method, and the parameters need documenting too.

I think we need to wait on this patch until #2599524: Fixing order of documentation sections for /core/lib/Drupal is done, because it will conflict. Right? Either that or postpone the other one.

Also, when we get back to this, the sections of additional documentation in the Tables class need a lot more work... basically the docs are not telling me what the parameters really are. For example:

1. +++ b/core/lib/Drupal/Core/Entity/Query/Sql/Tables.php
   @@ -234,8 +234,24 @@ public function isFieldCaseSensitive($field_name) {
   +   * @param string $index_prefix
   +   *   Table prefix name.
   

   Huh, that is odd. So the parameter is called $index_prefix and it is the prefix for a table? What does this mean?

2. +++ b/core/lib/Drupal/Core/Entity/Query/Sql/Tables.php
   @@ -234,8 +234,24 @@ public function isFieldCaseSensitive($field_name) {
       * @param string $property
   +   *   Field name.
   

   $property is a field name for what?

3. +++ b/core/lib/Drupal/Core/Entity/Query/Sql/Tables.php
   @@ -234,8 +234,24 @@ public function isFieldCaseSensitive($field_name) {
   +   * @param string $id_field
   +   *   Field name.
   

   This has the same documentation as $property above. That is not OK. What is the difference? Both need better docs.

4. +++ b/core/lib/Drupal/Core/Entity/Query/Sql/Tables.php
   @@ -234,8 +234,24 @@ public function isFieldCaseSensitive($field_name) {
   +   * @param array $entity_tables
   +   *   Array of tables.
   

   Array of what tables?

5. +++ b/core/lib/Drupal/Core/Entity/Query/Sql/Tables.php
   @@ -234,8 +234,24 @@ public function isFieldCaseSensitive($field_name) {
       * @return string
   +   *   Returns table alias.
   

   We don't normally start @return docs with
   "Returns".

   Just say "The table alias".

Note that the other method with added docs has the same problems.

Title was bugging me. Apostrophe is possession, not plural. ;)

That patch is in.

@here,

I am not able to apply the 2601282-2.patch. So i created the new one, which is according to the @jhodgdon comment #8. Please review the attached patch.

Sorry, but I still do not think that the @param documentation in this latest patch is accurately describing what the parameters are.

For instance, when I look at $index_prefix, the patch says it is "Index to prefix with the table name.". Besides the fact that I do not even understand what that is really supposed to mean, it's definitely not an index and it definitely is not prefixed with the table name. What it really is, looking at the code, is a string that for some unknown (to me) reason, is being added as a prefix to the table name when the table is added to the $entityTables member variable. The previous patch, which said "Table prefix name" was at least closer to correct, but it still didn't tell me what I would need to pass in for this value. I really have no idea what the correct documentation would be -- looking at the code in this function, it's not obvious. I'd need to look at where it was called and then how the $entityTables member variable was used elsewhere, in order to figure it out.

Similarly, the other parameter documentation I looked at, for the next few parameters, is either not understandable or wrong.

So, someone needs to carefully read through the code here, including looking at where these functions are called and what is passed in by the calling functions and how the saved values are used later, and figure out what the parameters really are, how they are used, and what their values should be. Then document them appropriately.

The index_prefix is documented in addField (my bad, should've been a param doxygen):

    // This variable ensures grouping works correctly. For example:
    // ->condition('tags', 2, '>')
    // ->condition('tags', 20, '<')
    // ->condition('node_reference.nid.entity.tags', 2)
    // The first two should use the same table but the last one needs to be a
    // new table. So for the first two, the table array index will be 'tags'
    // while the third will be 'node_reference.nid.tags'.

@chx,
Thanks for the help in IRC.

@jhodgdon
According to the discussion with @chx , Done couple of changes.

OK.... So let's look at this whole documentation block. The first line says:

Join entity table if necessary and return the alias for it.

There is nothing in there about fields or relationships or anything. Then the proposed docs launch into talking about fields and relationships and etc, and I have no context for understanding what it is talking about.

So we need more explanation about what this method does and what it is for. The first line, which is all we have for an overview, just says it is joining an entity table, but the rest is about... I have no idea. All of the proposed param docs seem to presume some knowledge or context that is not there at all in the docs. What is this method really for? Why are we joining the entity table? Why would it be necessary? What is the context?

Here are my specific confusions, plus a specific suggestion about the prefix parameter:

1. +++ b/core/lib/Drupal/Core/Entity/Query/Sql/Tables.php
   @@ -234,9 +234,34 @@ public function isFieldCaseSensitive($field_name) {
   +   *   Table name prefix to discern the same table used in several
   +   *   relationships.
   +   *   Example:
   +   *   When its empty.
   +   *   @code
   +   *   ->condition('tags', 2, '>')
   +   *   @endcode
   +   *   When it is node_reference.nid.
   +   *   @code
   +   *   ->condition('node_reference.nid.entity.tags', 2)
   +   *   @endcode
   

   The first sentence here contradicts the examples. The sentence says it is a prefix for the table name. The examples are showing it as being a prefix for field names.

   I think the correct answer is that it is a prefix for table names.

   The examples formatting is also not great... I think we can leave them out.

   How about changing this to just:

   Prefix that will be put on this table in queries, when it is used within this relationship.

   and leaving off the examples, which I think just confuse me.

2. +++ b/core/lib/Drupal/Core/Entity/Query/Sql/Tables.php
   @@ -234,9 +234,34 @@ public function isFieldCaseSensitive($field_name) {
       * @param string $property
   +   *   Mapped field table name.
   

   What does this mean? I don't know what "$property" should be in the context of "Join entity table". And "Mapped field table name" doesn't seem to related to "$property" at all, and besides which, I don't know what "mapped" means or what "field table" is. The context of this needs to be explained.

3. +++ b/core/lib/Drupal/Core/Entity/Query/Sql/Tables.php
   @@ -234,9 +234,34 @@ public function isFieldCaseSensitive($field_name) {
   +   * @param string $base_table
   +   *   Base table name.
   

   What does this mean? Base table for what?

4. +++ b/core/lib/Drupal/Core/Entity/Query/Sql/Tables.php
   @@ -234,9 +234,34 @@ public function isFieldCaseSensitive($field_name) {
   +   * @param string $id_field
   +   *   Field name.
   

   What field name, and why is it "field name" when the parameter is $id_field? How is it different from $property? What is the context here -- all I know is I'm checking to see if some entity table needs to be joined, and suddenly there is a field ID?

5. +++ b/core/lib/Drupal/Core/Entity/Query/Sql/Tables.php
   @@ -234,9 +234,34 @@ public function isFieldCaseSensitive($field_name) {
   +   * @param array $entity_tables
   +   *   A two dimensional array, the first is the name of a base or data table,
   +   *   the second is a column name.
   

   Two-dimensional array... what are the values and the keys? This is confusing. Especially it is confusing because there is a class member variable also called entityTables -- how is this related? What is it used for?

I just committed #2604722: Comment typo in BaseFieldDefinition.php file. Note that a better way of scoping these two issues would be to have:

• one patch that fixes all the spelling errors that you have found, i.e. the patch that was committed plus those simple typo fixes from this patch (since that is a single conceptual scope of issue), and
• a separate for adding the missing documentation, since reviewing that takes more work and requires more extensive review of a different nature, including actually examining the updated code.

That is easier to manage than different types of fixes in the same files/directories/etc.

Thanks for your contributions on these documentation issues! It's great to see all the improvements happening during the release candidate phase.

@xjm,

I have created the new issue only for the typo, https://www.drupal.org/node/2605264

this will continue for the only doc block , I will update issue summary too.

> The first sentence here contradicts the examples. The sentence says it is a prefix for the table name. The examples are showing it as being a prefix for field names.

There's no contradiction: it is a table prefix based the relationship specifier and the latter is the the name of column containing the relationship . For example, author.uid. will be a prefix, that doesn't make it a prefix for field names,it's just based on something looking like a field name but it's not , it's a relationship specifier.

Changes done in patch.
As mentioned in comment #21
1. Changed to "Prefix that will be put on this table in queries, when it is used within this relationship."
2. Changed to "Field name for mapping with base table.".
3. Changed to "Base table name,the base table based on where it finds the $property first".
4. Changed to "This contains the relevant SQL field name to be used when joining entity tables"
5. Changed to "A two dimensional array, table name as key (base or data table) and array of column name as value of the respective table."

Thanks for the patch, but it really needs some work... maybe go back to the previous patch and start over...

1. +++ b/core/lib/Drupal/Core/Entity/Query/Sql/Tables.php
   @@ -234,9 +234,26 @@ public function isFieldCaseSensitive($field_name) {
   +   *   Prefix that will be put on this table in queries, when it is used within
   +   *   this relationship.
   

   So again, what is "this relationship" that is being discussed? All I know before I get to this line is that this function joins the entity table and returns an alias. Now suddenly we are talking about a relationship, and I do not know what this means. Maybe it should just say "join" instead of "relationship"?

   Also, this variable is called $index_prefix, why "index" in the name if it has nothing to do with indexes?

   And why "queries", when presumably it is just for one query?

2. +++ b/core/lib/Drupal/Core/Entity/Query/Sql/Tables.php
   @@ -234,9 +234,26 @@ public function isFieldCaseSensitive($field_name) {
   +   *   Field name for mapping with base table.
   

   What does this mean?

3. +++ b/core/lib/Drupal/Core/Entity/Query/Sql/Tables.php
   @@ -234,9 +234,26 @@ public function isFieldCaseSensitive($field_name) {
   +   *   Base table name,the base table based on where it finds the $property first
   

   This is unclear, and it needs to end in a . and be wrapped at 80 character lines.

4. +++ b/core/lib/Drupal/Core/Entity/Query/Sql/Tables.php
   @@ -234,9 +234,26 @@ public function isFieldCaseSensitive($field_name) {
   +   *   This contains the relevant SQL field name to be used when joining entity
   +   *   tables.
   

   for which of the join parts?

5. +++ b/core/lib/Drupal/Core/Entity/Query/Sql/Tables.php
   @@ -234,9 +234,26 @@ public function isFieldCaseSensitive($field_name) {
   +   *   A two dimensional array, table name as key (base or data table) and
   +   *   array of column name as value of the respective table.
   

   This is garbled.

6. +++ b/core/lib/Drupal/Core/Entity/Query/Sql/Tables.php
   @@ -255,9 +272,25 @@ protected function ensureEntityTable($index_prefix, $property, $type, $langcode,
   +   *   Prefix that will be put on this table in queries, when it is used within
   +   *   this relationship.
   

   See above.

7. +++ b/core/lib/Drupal/Core/Entity/Query/Sql/Tables.php
   @@ -255,9 +272,25 @@ protected function ensureEntityTable($index_prefix, $property, $type, $langcode,
   +   * @param object $field
   

   Really, it's a generic "object" and not a specific class?

8. +++ b/core/lib/Drupal/Core/Entity/Query/Sql/Tables.php
   @@ -255,9 +272,25 @@ protected function ensureEntityTable($index_prefix, $property, $type, $langcode,
   +   *   A field object.
   

   Be more specific. What field object?

9. +++ b/core/lib/Drupal/Core/Entity/Query/Sql/Tables.php
   @@ -255,9 +272,25 @@ protected function ensureEntityTable($index_prefix, $property, $type, $langcode,
   +   *   Base table name,the base table based on where it finds the $property first.
   

   garbled, unclear, I do not know what this means, and what is $property anyway?

   also more than 80 characters

10. +++ b/core/lib/Drupal/Core/Entity/Query/Sql/Tables.php
    @@ -255,9 +272,25 @@ protected function ensureEntityTable($index_prefix, $property, $type, $langcode,
    +   * @param string $entity_id_field
    +   *   Either entity field ID value, or revision field ID value.
    +   * @param array $field_id_field
    +   *   Whether it is entity ID, or revision ID.
    

    These two params have the same name? And I don't understand what they mean.

11. +++ b/core/lib/Drupal/Core/Entity/Query/Sql/Tables.php
    @@ -255,9 +272,25 @@ protected function ensureEntityTable($index_prefix, $property, $type, $langcode,
    +   *   Either entity field ID value, or revision field ID value.
    

    value for what?

12. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
    @@ -155,10 +155,13 @@ public function getFieldStorageDefinitions() {
    +   *   Defines an interface for cache implementations.
    

    No. We're not defining an interface, we're passing something in for a @param.

13. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
    @@ -155,10 +155,13 @@ public function getFieldStorageDefinitions() {
    +   * @internal param CacheBackendInterface $cache_backend
    

    What is this @internal thing?

@snehi, Can you please update the status on this ?

2601282-4.patch -> failed. Needs reroll

I'd like to give this a go, starting with re-roll of 10739450-4.patch

used bisect to re-roll 2601282-4a.patch.

no conflicts, auto merge.

So I think next steps is to work on the issue.

That patch has a lot of interesting stuff in it that doesn't belong, like

+@@ -234,9 +234,34 @@ public function isFieldCaseSensitive($field_name) {
+   /**
+    * Join entity table if necessary and return the alias for it.
+    *
++   * @param string $index_prefix
++   *   Table name prefix to discern the same table used in several

etc.

Hows about it.

Please read the previous reviews before making a patch and setting the status to "Needs review". They still have not been addressed.

To be clear: I am still having a lot of problems with this documentation. Looking at the first function... (as noted at the top of #21 review as well as the #29 review), the first line docs say:

   /**
    * Join entity table if necessary and return the alias for it.

Then a lot of the @param docs start talking about relationships and fields, but there is no context for understanding what they are talking about, since all we have above that is "Join entity table if necessary".

So we really really need to add something to the documentation above the @params that explains what field/relationship we are talking about, before having param docs. They don't make any sense unless you already know something that is not in the documentation. Here are some examples of confusion I got when reading this method documentation:

+   * @param string $index_prefix
+   *   Prefix that will be put on this table in queries, when it is used within
+   *   this relationship.

What is "this relationship"? First I've heard of a relationship.

   * @param string $property
+   *   Mapped field table name.

What field? What is a "mapped field table name" anyway?

+   * @param string $base_table
+   *   Base table name.

What does "base table name" mean?

+   * @param string $id_field
+   *   Field name.

What field?

So this documentation is not making anything clear to me. The purpose of documentation of a method is to explain what the method does, and what it is used for. This documentation just leaves me with questions. I still have no idea what these methods are for.

@jhodgdon, I have read and reread several times all comments/doc in /drupal/core/lib/Drupal/Core/Entity/Query/Sql/Tables.php and I believe most of the questions you rightfully raised over the lack of context and the abundance of inferences have been addressed in a meaningful way since this thread went silent 4 years ago.

I think that being mostly a site-builder made me a good candidate/reader i.e. every inference sent me googling, so I learned a few things in the process. I also took the liberty to make a few changes that helped me better understand what's going on.

I hope you'll consider reviewing the attached patch and help me bring it to a state where it can be committed.

P.s. There are 2 TODOs in the patch, can someone help with those?

After reading User interface standards, I went through one more time to apply the following rules:

• Do not use the pronoun "we".
• Do not use contractions, like "you've", "can't", and "shouldn't".

New patch is attcheed.

Little modifications to the patch. Adding a comma before the word 'then' in several places.

if ($key < $count) {
           $next = $specifiers[$key + 1];
-          // If this is a numeric specifier we're adding a condition on the
+          // If this is a numeric specifier, then add a condition on the
           // specific delta.
           if (is_numeric($next)) {
             $delta = $next;
@@ -151,7 +152,7 @@ public function addField($field, $type, $langcode) {
             $key++;
             $next = $specifiers[$key + 1];
           }

-          // If this specifier is the reserved keyword "%delta" we're adding a
+          // If this specifier is the reserved keyword "%delta", then add a
           // condition on a delta range.

@@ -227,7 +228,7 @@ public function addField($field, $type, $langcode) {
         // next one is a column of this field.
         if ($key < $count) {
           $next = $specifiers[$key + 1];
-          // If this specifier is the reserved keyword "%delta" we're adding a
+          // If this specifier is the reserved keyword "%delta", then add a
           // condition on a delta range.
           if ($next == TableMappingInterface::DELTA) {
             $key++;
@@ -238,9 +239,9 @@ public function addField($field, $type, $langcode) {
               return 0;
             }
           }

// If this is a numeric specifier we're adding a condition on the
-          // specific delta. Since we know that this is a single value base
-          // field no other value than 0 makes sense.
+          // If this is a numeric specifier, then add a condition on the
+          // specific delta. Since this is a single value base field, the only
+          // value that makes sense is 0.

TODO 1: If there exists both a field storage and a field column, check for case sensitivity.

TODO 2: Follows here attempts to state that "for data tables, the value of $langcode_key is retrieved from the $entity_type object while for other table types, $landcode_key = 'langcode' "

Hope this clears it.

Hello @emyu01,

Done with comma modifications.
Kindly review patch.

Hi neelam_wadhwani,

Your patch does not apply cleanly because you will need to also take care of the TODOs specified in #53 which i have offered solutions in #55. They occur on lines 267 and 420 respectively. As an alternative, since the errors seem to be of different nature, we may remove the TODOs from current patch and fix them in a different issue entirely.

@emyu01 , there was a whitespace at line number 421, so it didn't got applied

@Neslee you're right. I will just include the TODO fixes I suggested earlier.

Here I have included both TODO fixes. kindly review.

+++ b/core/lib/Drupal/Core/Entity/Query/Sql/Tables.php
@@ -142,7 +143,7 @@ public function addField($field, $type, $langcode) {
-          // If this is a numeric specifier we're adding a condition on the
+          // If this is a numeric specifier, then add a condition on the
           // specific delta.
           if (is_numeric($next)) {
             $delta = $next;
@@ -151,7 +152,7 @@ public function addField($field, $type, $langcode) {

@@ -151,7 +152,7 @@ public function addField($field, $type, $langcode) {
             $key++;
             $next = $specifiers[$key + 1];
           }
-          // If this specifier is the reserved keyword "%delta" we're adding a
+          // If this specifier is the reserved keyword "%delta", then add a
           // condition on a delta range.

I don't think these changes are correct.

"If ... we're" is about deducing what the current intent of the method call is.

"If ... then" is about what the code is about to do.

Those are different concepts.

Patch can't be applied on 8.9.x.Needs reroll.

The reroll is suitable for a novice, keeping the tag.

I have re-rolled the the patch from #62 to 8.9.x and included a re-roll interdiff as well. Tested also the new patch and it applied cleanly.

LGTM.

Removing Needs reroll tag as it is no longer needed.

Needs a reroll again.

Rerolled the patch against 9.4.x branch.

Updated patch with interdiff.

I read the IS and then skimmed the patch.

This patch is not fixing the problem stated in the Issue Summary. I think that problem was fixed in #2068655: Entity fields do not support case sensitive queries in 8.0.x. What is done here are improvements to the documentation in a single file, \Drupal\Core\Entity\Query\Sql\Tables, a file not mentioned in the Issue Summary.

Now looking at the patch more closely and found the following items.

1. +++ b/core/lib/Drupal/Core/Entity/Query/Sql/Tables.php
   @@ -80,13 +80,14 @@ public function __construct(SelectInterface $sql_query) {
   +    // This variable ensures grouping works correctly. For example,
   +    // given the following conditions:
   

   This is not wrapped correctly.

2. +++ b/core/lib/Drupal/Core/Entity/Query/Sql/Tables.php
   @@ -110,9 +111,9 @@ public function addField($field, $type, $langcode) {
   +      // Where there is revision support, only the current revisions are being
   

   This is a change from the original. Is this really only getting the current revision or all revisions?

I think this is a Task because it is improving the readability of the documentation, and cleanup such as expanding contractions and spacing so I think this is a task. However, there is one question, in the feedback above that needs to be answered before changing the category.

This really needs an Issue Summary update, adding tag. See Write an issue summary for an existing issue for guidance.

Leaving novice tag because updating this issue summary is a suitable for a first issue for someone.

@yogeshmpawar
patch is not applying in drupal9.3 going to skipping for ref sharing screenshot ....

Added fix for #74.1
Point number #74.2 still needs a fix.

@vikashsoni, @rishabh064, thanks for the interest in this issue. Your work here shows that you have not read the previous comment or the issue tags to find out what work needs to be done here. That just adds noise to the issue and work for others. So, no credit will be applied.

It has been two years since I asked for an issue summary update and it has not happened. I have opted to do so myself and create an MR.

@vikashsoni, @rishabh064, thanks for the interest in this issue. Your work here shows that you have not read the previous comment or the issue tags to find out what work needs to be done here. That just adds noise to the issue and work for others. So, no credit will be applied.

It has been two years since I asked for an issue summary update and it has not happened. I have opted to do so myself and create an MR.

Super nitpicky change to one sentence thoughts?

Applied the change directly, not sure when the gitlab fix was pushed so we could edit MRs opened by committers but yay!

My change was so small don't mind marking, as rest looks fine.

Committed fa5921b and pushed to 11.x. Thanks!

Crediting myself for mentoring and scope guidance in #22.