Problem/Motivation

The definition of the {users_data} table in user_schema() includes this for the foreign_keys key:

'foreign keys' => [
  'uid' => ['users' => 'uid'],
]

This is not consistent with the structure of that key as described in the Schema API page in the API docs.

Proposed resolution

Change those lines to be consistent with the API docs. Several other modules define foreign_keys correctly. The {node_access} table, defined in node_schema() is a good model.

Remaining tasks

User interface changes

None

API changes

None

Data model changes

None

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

benjifisher created an issue. See original summary.

benjifisher’s picture

The current patch on #2883260: Replace the schema example with one actually used from a module uses the {users_data} table as the example in database.api.php, which is the source of the Schema API documentation. I am recommending a similar change there.

apaderno’s picture

Issue summary: View changes
apaderno’s picture

Title: user_schema() has wrong structure for foreign_keys » user_schema() uses the wrong structure for foreign_keys
apaderno’s picture

Status: Active » Needs review
FileSize
596 bytes
benjifisher’s picture

Status: Needs review » Needs work

@kiamlaluno:

Thanks for looking at this, and for updating the issue title!

+    // For documentation purposes only; foreign keys are not created in the
+    // database.

I know that @alexpott, in [#2883260.11], says to keep that comment in the sample code in database.api.php, but I still think it is redundant. In this context, I can also argue for consistency: none of the other core modules have such a comment in their code.

+      'data_owner' => [

I guess that the key can be anything, but why "data_owner"? Do you mean that this is the users_data table, and this key identifies the user that "owns" the data in this table?

Looking at the other core modules,

  • Some use the table name as the primary key in the foreign_keys array; here, that would be users. I do not like that much.
  • The Shortcut module uses set_user, so maybe data_user would follow the same pattern.
  • Similarly, the sessions table in the System module uses session_user.
  • The node_access table uses affected_node, so affected_user would follow that pattern.

I suggest data_user or affected_user, but you can leave it as data_owner if you think that is clearest.

apaderno’s picture

Status: Needs work » Needs review
FileSize
469 bytes

affected_node makes sense for the node access table, but for the user_data table I see preferable the pattern used for the sessions table.

I left the comment because it was already present in the example given for the schema API. It probably doesn't make sense to repeat it for the module, where it doesn't have the purpose of "teaching" about the foreign keys being used only for documentation.

benjifisher’s picture

Status: Needs review » Reviewed & tested by the community

@kiamlaluno:

Thanks for the update. This looks good to me.

apaderno’s picture

@benjifisher Thank you for the suggestions.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed 125e9f4 and pushed to 8.7.x. Thanks!

  • catch committed 125e9f4 on 8.7.x
    Issue #3011237 by kiamlaluno, benjifisher: user_schema() uses the wrong...

Status: Fixed » Closed (fixed)

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