Replace the schema example with one actually used from a module

Needs a re-roll

Hi,
I just tried this and it still applies cleanly with 8.6.x and it looks ok to me. Marking as RTBC

Tests pass, patch applies cleanly, just working with comments here, and the example looks pretty good.

+++ b/core/lib/Drupal/Core/Database/database.api.php
@@ -328,52 +328,53 @@
- *   // For documentation purposes only; foreign keys are not created in the
- *   // database.
...
+ *    'foreign keys' => [
+ *      'uid' => ['users' => 'uid'],
+ *    ],

I think it is worth maintaining the comment.

Thanks for the patch, kiamlaluno. Just a couple of notes. The paragraph before the array still says that this is an example for the 'nodes' table, which needs to be updated along with other text as to what fields/primary keys are listed. Also, every changed line after "* $schema['users_data'] = [" is indented one space too many.

Changes addressed mentioned in comment #13 with an interdiff.

Thanks Yogesh. Looks good to me.

More stuff to fix :(

 *   - 'description': A string in non-markup plain text describing this table
 *     and its purpose. References to other tables should be enclosed in
 *     curly-brackets. For example, the node_field_revision table
 *     description field might contain "Stores per-revision title and
 *     body data for each {node}."
 *   - 'fields': An associative array ('fieldname' => specification)
 *     that describes the table's database columns. The specification
 *     is also an array. The following specification parameters are defined:
 *     - 'description': A string in non-markup plain text describing this field
 *       and its purpose. References to other tables should be enclosed in
 *       curly-brackets. For example, the node table vid field
 *       description might contain "Always holds the largest (most
 *       recent) {node_field_revision}.vid value for this nid."

Instead of using the node_field_revision example let's change these to use the users_data example too. Whilst we won't get a curly bracket in the table description we will in the user_data.uid example.

I also think we should update the hook_schema() example in database.api.php too.

@kiamlaluno:

Thanks for updating the patch. It looks as though you settled on the tracker_node table from the tracker module.

--- a/core/lib/Drupal/Core/Database/database.api.php
+++ b/core/lib/Drupal/Core/Database/database.api.php
@@ -249,17 +249,15 @@
  * The following keys are defined:
  *   - 'description': A string in non-markup plain text describing this table
  *     and its purpose. References to other tables should be enclosed in
- *     curly-brackets. For example, the node_field_revision table
- *     description field might contain "Stores per-revision title and
- *     body data for each {node}."
+ *     curly-brackets. For example, a description for the tracker_node table
+ *     could be "Tracks when {nodes} were last changed or commented on."

There are a few problems with this example:

• Although the tracker_node could use that description, the actual description does not include the curly brackets.
• The table is {node}, not {nodes}. I do not like using the plural this way, and I do not like {node}s any better.
• I prefer to use an example from the {user} module, since that is much more familiar than the {tracker} module.

I appreciate the effort you spent finding this example, but I would prefer going back to the patch in #15 and making the changes requested in #17.

Here is the updated patch according to #22.

Since #2886740: user_schema() reports uid as primary key for the user_data table when it's just part of the primary key updates the description of the uid key in the {users_data} table, we should update the code here if that issue is accepted.

@msankhala:

Thanks for moving forward with this. I think a few things should be changed.

1. +++ b/core/lib/Drupal/Core/Database/database.api.php
   @@ -249,17 +249,14 @@
   + *       curly-brackets. For example, the users_data table uid field
   + *       description might contain "Primary key: {users}.uid for user."
   

   Let's pay attention to #2886740: user_schema() reports uid as primary key for the user_data table when it's just part of the primary key. If that issue is fixed, then we should make the corresponding update here.

2. @@ -327,59 +324,61 @@
     * of the named column.
     *
     * As an example, here is a SUBSET of the schema definition for
   - * Drupal's 'node' table. It show four fields (nid, vid, type, and
   

   Please remove the reference to SUBSET, since we are copying in the entire definition of the {users_data} table.

3. @@ -327,59 +324,61 @@
   ...
     *   // For documentation purposes only; foreign keys are not created in the
     *   // database.
   

   I do not think this code comment is helpful here. It repeats what is said before the code sample, and it is not part of the actual code from user_install().

4. @@ -327,59 +324,61 @@
   ...
   + *   'foreign keys' => [
   + *     'uid' => ['users' => 'uid'],
   + *   ],
   + * ];
     * @endcode
   

   This is very odd. This is not consistent with the documentation that comes before the code sample. I checked the implementations of hook_install() in all the other core modules, and this is the only one with the wrong structure for this key. I added #3011237: user_schema() uses the wrong structure for foreign_keys to fix this in the User module, but I think we should not wait for that issue to fix it here. Please use the structure in node_install() as a model.

5. @@ -490,60 +489,56 @@ function hook_query_TAG_alter(Drupal\Core\Database\Query\AlterableInterface $que
   

   I think all the comments above apply twice, since the same code appears inside the @code annotations and then as part of hook_schema().

Update: I see that @alexpott's comment in #11 disagrees with my suggestion (3). I defer to @alexpott, so please leave the code comment in.

I think that this issue and #2886740 can be done in either order. Whichever is fixed second can make sure that the example is consistent with the actual code.

Thanks for updating the title of this issue. I think it is clearer.

Now #2886740: user_schema() reports uid as primary key for the user_data table when it's just part of the primary key has been fixed, so we should definitely make the change I suggested in #25.1.

Now #3011237: user_schema() uses the wrong structure for foreign_keys has been fixed, too, so we should make the change I suggested in #25.4.

@kiamlaluno:

Now that we have taken care of the related issues, we can handle this one. Yay! Thanks for the update.

I have attached an interdiff, comparing the patches in #23 and #31. Usually, we ask the author of the new patch to supply these. If you need instructions for creating an interdiff, see Creating an interdiff.

I have a couple of complaints about this hunk:

@@ -326,60 +323,66 @@
  * array of two elements, column name and length, specifying a prefix
  * of the named column.
  *
- * As an example, here is a SUBSET of the schema definition for
- * Drupal's 'node' table. It show four fields (nid, vid, type, and
- * title), the primary key on field 'nid', a unique key named 'vid' on
- * field 'vid', and two indexes, one named 'nid' on field 'nid' and
- * one named 'node_title_type' on the field 'title' and the first four
- * bytes of the field 'type':
+ * As an example, this is the schema definition for the Drupal's users_data
+ * table. It shows four fields (uid, module, name, value, and serialized),
+ * the primary key (on the uid, module, and name fields), and two indexes (one
+ * named module on the module field, and one named name on the name field).

First, it should be "for Drupal's users_data table", not "for the Drupal's users_data table".

Second, you have removed all the single quotes. If you think that is a good idea, then please explain why instead of just doing it. I do not feel strongly about it, although "one named name on the name field" (without any quotation marks) seems a little odd. As a general rule, I try to avoid changing the existing style unless I think it makes a big difference.

@@ -326,60 +323,66 @@
...
+ *   'foreign keys' => [
+ *     'data_user' => [
+ *      'table' => 'users',
+ *      'columns' => [
+ *        'uid' => 'uid',
+ *      ],
+ *    ],
+ *  ],

Please fix the indentation.

@kiamlaluno:

Thanks. The use of quotation marks is now consistent, and I think that the parentheses make it clearer.

Thanks also for fixing the indentation.

I have the advantage of being a native English speaker, and I am quite sure that "the Drupal's 'users_data' table" is incorrect. Either "Drupal's 'users_data' table" or "the 'users_data' table" would be fine.

I will ask you to reconsider one more time. If we cannot agree, then I am willing to move this issue to RTBC and let the core committers decide.

@kiamlaluno:

If we can agree to compromise, that is good enough for me. Thanks for making it "the 'users_data' table". And thanks for introducing me to the Corpus of Contemporary American English. It is an interesting resource. It took me a while to find the correct syntax for the search you described: one way is "for the NAME 's NOUN".

@@ -322,64 +319,70 @@
...
+ * As an example, this is the schema definition for the 'users_data' table. It
+ * shows four fields ('uid', 'module', 'name', 'value', and 'serialized'), the

I am sorry for not catching this earlier, but please correct the off-by-one error.

As long as you are doing that, you can make the changes you suggested in #37 if you like.

@kiamlaluno:

If it is fine, I can make this issue more generic to fix typos and formatting for that documentation page. Do you think it is doable?

We should try to stay in scope. The hyphen in "curly-bracket" annoys me, so I hope the core committers will allow this fix.

Thanks for your patience. I like this version.

Committed and pushed b9c85bd428 to 8.7.x and 74542fac12 to 8.6.x. Thanks!