The example of schema given in the Schema API documentation uses code that the Node module doesn't use anymore, since node_schema() doesn't define anymore a node table.

$schema['node'] = array(
  'description' => 'The base table for nodes.',
  'fields' => array(
    'nid' => array(
      'type' => 'serial',
      'unsigned' => TRUE,
      'not null' => TRUE,
    ),
    'vid' => array(
      'type' => 'int',
      'unsigned' => TRUE,
      'not null' => TRUE,
      'default' => 0,
    ),
    'type' => array(
      'type' => 'varchar',
      'length' => 32,
      'not null' => TRUE,
      'default' => '',
    ),
    'language' => array(
      'type' => 'varchar',
      'length' => 12,
      'not null' => TRUE,
      'default' => '',
    ),
    'title' => array(
      'type' => 'varchar',
      'length' => 255,
      'not null' => TRUE,
      'default' => '',
    ),
    'uid' => array(
      'type' => 'int',
      'not null' => TRUE,
      'default' => 0,
    ),
    'status' => array(
      'type' => 'int',
      'not null' => TRUE,
      'default' => 1,
    ),
    'created' => array(
      'type' => 'int',
      'not null' => TRUE,
      'default' => 0,
    ),
    'changed' => array(
      'type' => 'int',
      'not null' => TRUE,
      'default' => 0,
    ),
    'comment' => array(
      'type' => 'int',
      'not null' => TRUE,
      'default' => 0,
    ),
    'promote' => array(
      'type' => 'int',
      'not null' => TRUE,
      'default' => 0,
    ),
    'moderate' => array(
      'type' => 'int',
      'not null' => TRUE,
      'default' => 0,
    ),
    'sticky' => array(
      'type' => 'int',
      'not null' => TRUE,
      'default' => 0,
    ),
    'translate' => array(
      'type' => 'int',
      'not null' => TRUE,
      'default' => 0,
    ),
  ),
  'indexes' => array(
    'node_changed' => array(
      'changed',
    ),
    'node_created' => array(
      'created',
    ),
    'node_moderate' => array(
      'moderate',
    ),
    'node_frontpage' => array(
      'promote',
      'status',
      'sticky',
      'created',
    ),
    'node_status_type' => array(
      'status',
      'type',
      'nid',
    ),
    'node_title_type' => array(
      'title',
      array(
        'type',
        4,
      ),
    ),
    'node_type' => array(
      array(
        'type',
        4,
      ),
    ),
    'uid' => array(
      'uid',
    ),
    'translate' => array(
      'translate',
    ),
  ),
  'unique keys' => array(
    'vid' => array(
      'vid',
    ),
  ),
  // For documentation purposes only; foreign keys are not created in the
  // database.
  'foreign keys' => array(
    'node_revision' => array(
      'table' => 'node_field_revision',
      'columns' => array(
        'vid' => 'vid',
      ),
    ),
    'node_author' => array(
      'table' => 'users',
      'columns' => array(
        'uid' => 'uid',
      ),
    ),
  ),
  'primary key' => array(
    'nid',
  ),
);

The example code should be one an existing module is actually using.

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

kiamlaluno created an issue. See original summary.

kiamlaluno’s picture

The following code could be used. It shows foreign keys, the primary key, and indexes.

  $schema['users_data'] = [
    'description' => 'Stores module data as key/value pairs per user.',
    'fields' => [
      'uid' => [
        'description' => 'Primary key: {users}.uid for user.',
        'type' => 'int',
        'unsigned' => TRUE,
        'not null' => TRUE,
        'default' => 0,
      ],
      'module' => [
        'description' => 'The name of the module declaring the variable.',
        'type' => 'varchar_ascii',
        'length' => DRUPAL_EXTENSION_NAME_MAX_LENGTH,
        'not null' => TRUE,
        'default' => '',
      ],
      'name' => [
        'description' => 'The identifier of the data.',
        'type' => 'varchar_ascii',
        'length' => 128,
        'not null' => TRUE,
        'default' => '',
      ],
      'value' => [
        'description' => 'The value.',
        'type' => 'blob',
        'not null' => FALSE,
        'size' => 'big',
      ],
      'serialized' => [
        'description' => 'Whether value is serialized.',
        'type' => 'int',
        'size' => 'tiny',
        'unsigned' => TRUE,
        'default' => 0,
      ],
    ],
    'primary key' => ['uid', 'module', 'name'],
    'indexes' => [
      'module' => ['module'],
      'name' => ['name'],
    ],
    'foreign keys' => [
      'uid' => ['users' => 'uid'],
    ],
  ];
kiamlaluno’s picture

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

joshmiller’s picture

Status: Needs review » Needs work

Needs a re-roll

benjifisher’s picture

Issue tags: +Needs reroll, +Nashville2018, +Novice
kporras07’s picture

Status: Needs work » Reviewed & tested by the community

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

kiamlaluno’s picture

Status: Reviewed & tested by the community » Needs review

It should still apply, but let's see what the tester says.

joshmiller’s picture

Status: Needs review » Reviewed & tested by the community

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

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ 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.

kiamlaluno’s picture

Status: Needs work » Needs review
FileSize
4.14 KB
bdlangton’s picture

Status: Needs review » Needs work

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.

yogeshmpawar’s picture

Assigned: Unassigned » yogeshmpawar
yogeshmpawar’s picture

Assigned: yogeshmpawar » Unassigned
Status: Needs work » Needs review
FileSize
4.71 KB
3.81 KB

Changes addressed mentioned in comment #13 with an interdiff.

bdlangton’s picture

Status: Needs review » Reviewed & tested by the community

Thanks Yogesh. Looks good to me.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: -Needs reroll

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.

rakesh.gectcr’s picture

Issue tags: +Nwdug_may18

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

kiamlaluno’s picture

Issue summary: View changes
kiamlaluno’s picture

Status: Needs work » Needs review
FileSize
8.35 KB

I searched another schema that would reference another table in its description, it would reference another table's field in a field description, and that would use foreign keys. I have used that as example, also in the hook_schema() example given for the hook_schema() documentation.

benjifisher’s picture

Status: Needs review » Needs work

@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.

msankhala’s picture

Status: Needs work » Needs review
FileSize
8.72 KB
4.25 KB

Here is the updated patch according to #22.

benjifisher’s picture

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.

benjifisher’s picture

Status: Needs review » Needs work

@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.

kiamlaluno’s picture

Basing on #24, should not this issue be postponed? Since the fix in the other issue influences this issue, we should fix the other one first, and then this one. It would not make sense to change the example, and change it again once the other issue is fixed.

kiamlaluno’s picture

Title: Replace the schema example » Replace the schema example with one actually used from a module
Issue summary: View changes
benjifisher’s picture

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.

benjifisher’s picture

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.

benjifisher’s picture

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’s picture

Status: Needs work » Needs review
FileSize
8.98 KB
benjifisher’s picture

Status: Needs review » Needs work
FileSize
2.92 KB

@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’s picture

Status: Needs work » Needs review
FileSize
8.99 KB
3.09 KB

The article in that sentence is correct.
I didn't use any single quote character since the first part of the sentence didn't use them, so I kept that style.
I used parentheses because, in a list where its items contain commas, the comma should not be used to separate the list items. The alternatives are using semicolons to separate the list items, or using parentheses as I did.

benjifisher’s picture

@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.

benjifisher’s picture

Status: Needs review » Needs work
kiamlaluno’s picture

Status: Needs work » Needs review
FileSize
9.29 KB
3.39 KB

The Corpus of Contemporary American English shows many instances of of the <proper noun> 's <noun>.

I find Drupal unnecessary, since the documentation is for Drupal code. Even in the case Drupal would mean Drupal core (to make clear it's not a database schema used from third-party modules), I find it not necessary.

kiamlaluno’s picture

As side note, that should probably be removed from the following sentence.

hook_schema() should return an array with a key for each table that the module defines

A native speaker would probably say for each table the module defines. (At least, this is what I remember of American English.)

The following phrase could also be modified.

(one named 'module' on the 'module' field, and one named 'name' on the 'name' field)

I would rephrase it as the following.

(the 'module' index on the 'module' field and the 'name' index on the 'name' field)

Since the list of the database fields doesn't say one named, it doesn't make sense to me to say it for the indexes.

kiamlaluno’s picture

There are other parts that should be fixed, for example replacing curly-brackets with curly brackets, removing etc in a e.g. list, but these changes are not anymore about the example.

benjifisher’s picture

Status: Needs review » Needs work

@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’s picture

It took me a while too, since they changed their syntax, but I noticed there is a drop-down list that helps. ([POS], in the search page is clickable; once it is clicked, it is replaced by a button that clicked shows a list of categories.)

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?

Whoops... I didn't notice that too.

kiamlaluno’s picture

Status: Needs work » Needs review
FileSize
9.44 KB
3.91 KB
benjifisher’s picture

Status: Needs review » Reviewed & tested by the community

@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.

The last submitted patch, 40: change-schema-example-2883260-39.patch, failed testing. View results

kiamlaluno’s picture

What is #43 reporting? From https://www.drupal.org/pift-ci-job/1117612, I don't see any failed test. (Nevermind, it's for the patch I attached before noticing there were new comments, the one I then deleted.)

The last submitted patch, 15: 2883260-15.patch, failed testing. View results

kiamlaluno’s picture

Status: Reviewed & tested by the community » Needs review

I am re-testing the patch to understand why it is said to fail days after it was tested. If it doesn't fail, I will change back the status.

kiamlaluno’s picture

Status: Needs review » Reviewed & tested by the community

I re-tested the patch, and it passed all the tests again. I am setting the status back to Reviewed & tested by the community, which was the status set by @benjifisher in #42.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

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

  • alexpott committed b9c85bd on 8.7.x
    Issue #2883260 by kiamlaluno, yogeshmpawar, msankhala, benjifisher,...

  • alexpott committed 74542fa on 8.6.x
    Issue #2883260 by kiamlaluno, yogeshmpawar, msankhala, benjifisher,...

Status: Fixed » Closed (fixed)

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