For previous attempts at this, see http://drupal.org/node/28046 and http://drupal.org/node/79874. Unfortunately, those efforts failed because we were ham-strung by what MySQL supported. NOT ANYMORE! ;) Now we store meta data about our tables, and we are perfectly able to include a 'description' attribute on each field. For example:

function node_schema() {
  $schema['node'] = array(
    'fields' => array(
      'nid' => array(
        'type' => 'serial',
        'unsigned' => TRUE,
        'not null' => TRUE,
        'description' => t('Unique Node ID.'),
      ),
      'vid' => array(
        'type' => 'int',
        'unsigned' => TRUE,
        'not null' => FALSE,
        'default' => 0,
        'description' => t('Node Revision ID. References node_revisions.vid'),
      ),
      ...
    ),
  );

  return $schema;
}

Because this is just the addition of a simple attribute which core ignores, I don't think it qualifies as an API change, but rather as documentation. Let's kick it!

CommentFileSizeAuthor
#75 drupal-document-schema-164983-75.patch149.96 KBJirkaRybka
#74 drupal-document-schema-164983-72.patch155.54 KBbjaspan
#71 drupal-document-schema-164983-71.patch155.54 KBbjaspan
#69 drupal-document-schema-164983-69.patch151.64 KBadd1sun
#67 drupal-document-schema-164983-67.patch154.66 KBJirkaRybka
#64 drupal-document-schema-164983-64.patch154.57 KBadd1sun
#61 drupal-document-schema-164983-61.patch155.71 KBJirkaRybka
#60 drupal-document-schema-164983-60.patch155.68 KBwebchick
#57 drupal-document-schema-164983-54.patch153.85 KBpwolanin
#52 drupal-document-schema-164983-52.patch150.68 KBadd1sun
#51 drupal-document-schema-164983-51.patch150.06 KBadd1sun
#49 drupal-document-schema-164983-46.patch149.31 KByched
#45 drupal-document-schema-164983-45.patch148.96 KBadd1sun
#41 drupal-document-schema-164983-41.patch148.01 KBadd1sun
#39 drupal-document-schema-164983-39.patch147.34 KBadd1sun
#37 drupal-document-schema-164983-37.patch137.31 KBpwolanin
#35 drupal-document-schema-164983-35.patch139.19 KBbjaspan
#33 drupal-document-schema-164983-33.patch138.47 KBbjaspan
#32 drupal-document-schema-164983-32.patch106.93 KBJirkaRybka
#31 drupal-document-schema-164983-31.patch106.83 KBbjaspan
#30 drupal-document-schema-164983-30.patch106.45 KBbjaspan
#29 drupal-document-schema-164983-29.patch106.43 KBbjaspan
#24 drupal-document-schema-164983-24.patch86.47 KBpwolanin
#22 drupal-document-schema-164983-22.patch88.82 KBbjaspan
#21 drupal-document-schema-164983-21.patch81.57 KBadd1sun
#19 drupal-document-schema-164983-19.patch62.07 KBadd1sun
#18 drupal-document-schema-164983-18.patch52.5 KBwebchick
#14 drupal-document-schema-164983-14.patch51.08 KBwebchick
#10 drupal-document-schema-164983-10.patch51.12 KBwebchick
#9 drupal-document-schema-164983-9.patch42.59 KBwebchick
#8 drupal-document-schema-164983-8.patch34.34 KBwebchick
#7 drupal-document-schema-164983-7.patch28.52 KBwebchick
#3 drupal-document-schema-164983-3.patch20.29 KBwebchick
#2 drupal-document-schema-164983-2.patch12.92 KBwebchick
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

drumm’s picture

I think the API module would want to parse this documentation. As a general rule, API module never executes PHP code. Extracting this would be hard to do without either executing it or making some restrictive assumptions about the format.

I think we do need a generalized comment markup for big arrays, which would help with menu, form, link, and schema documentation. The usual use case is a bit different than schema documentation; usually we want to document example arrays, here we want to document the array itself.

webchick’s picture

That is true... however, I don't see a way around API module parsing PHP (or at least regexing it out) in order to document the database stuff, since it's all in arrays now. The advantage to doing things this way is we get to leverage the t() function, which means that we could have Dutch/German/etc. translations of the DB schema. Also, the documentation is right in-line with the schema code itself, so it's self-maintaining; we simply don't allow schema changes unless they come with documentation. :)

Here is an initial stab, with actions module and as much of aggregator module as I could figure out. Some notes:

- I added a description property both to the table itself and each field.
- Primary keys' description goes: t('Primary Key: Description of Key'), and foreign keys' descriptions go: t('Foreign Key: Description of Key').
- For foreign keys, I also added a 'foreign key' attribute, which takes an array containing field name, and an array of table, column. Like so:

    'foreign key' => array(
      'aid' => array('actions', 'aid'),
    ), 

I don't know if this is the optimal way of storing said information, but since we're going through the database with a fine-toothed comb, might as well tell it about this as well. We can always clean it up once schema API actually supports referential integrity.

webchick’s picture

Status: Active » Needs work
FileSize
20.29 KB

In a fit of insomnia, did the block.schema file too.

moshe weitzman’s picture

yes, we are ready now. +1 from me.

i'll go one step further and propose that db_create_table() should actually insert this documentation into the DB. then various tools including phpmyadmin can represent the data nicely.

for example, this statements adds a comment to an existing table. the same COMMENT keyword works for fields and it applies both to CREATE and ALTER statements. ALTER TABLE `access` COMMENT = 'my table comments'. mysql will happily truncate long descriptions so feel free to ramble on :)

i think we have to suck it up and do this in php as angie proposes. thats what schema uses.

bjaspan’s picture

Angie, this issue is a duplicate of http://drupal.org/node/160599 which I created three weeks ago when I added this capability to schema.module. I also announced it on the dev list but it got no attention. I guess you just get more respect than I do. :-) I'm tempted to mark this issue as a duplicate but there have been more posts here so I guess I'll mark my issue as a duplicate.

The schema.module now supports displaying documentation included in the schema data structure as specified in http://drupal.org/node/146939. When you are creating description fields that reference other tables, please wrap the table in curly-braces, like this:

   $schema['node_access'] = array(
     'description' => t('blah blah'),
     'fields' => array(
        'nid' => array('description' => t('The {node}.nid that this rule applies to.'), ...)

This way, the schema.module can (and does) display the table name as a hyperlink to its definition.

bjaspan’s picture

My guidance on foreign keys is not to include that information as part of this issue as it will just complicate it and delay getting the description fields in.

I worked on ref integrity a couple months ago, defined a way to represent foreign keys in the schema structure, and wrote the code in the pgsql driver to create the foreign keys. It was actually quite easy to do. However, it turns out that Drupal will need substantial and careful work before referential integrity can be added including changes all over core and contrib; for example, see my previous thread on this list about whether to use RESTRICT or CASCADE deletions and the impliciations for the rest of system. Since we cannot safely *use* foreign key info, I'm not sure it is a good idea to include it in the schema.

We could include foreign key info "as documentation" but we'd want to be sure we got the representation right, and it is tricky. First, the foreign keys array must be keyed by table name, not field name, to reasonably support multi-column foreign keys. Here is the format I came up with:

$schema['node'] = array(
  'fields' => array(
     'nid' => array('type' => 'int'),
     ...
  ),
);

$schema['node_revisions'] = array(
  'fields' => array(
     'nid' => array('type' => 'int'),
     'vid' => array('type' => 'int'),
     ...
  ),
  'foreign keys => array(
    'node' => array('nid' => 'nid'),
  ),
));

$schema['content_type_foobar'] = array(
   'fields' => array(
     'nid' => array('type' => 'int'),
     'vid' => array('type' => 'int'),
     ...
   ),
   'foreign keys' => array(

    // redundant, just shows you can have more than one:
    'node' => array('nid' => 'nid'),

    // could just be on vid, but shows you can have multiple columns:
    'node_revisions' => array('nid' => 'nid', 'vid' => 'vid'),
   ),
));

As shorthands, when the column name is the same in both tables you can just specify it as value, not as key => value, and when there is exactly one column in the foreign key and its name is the same in both tables, you can specify it as a string instead of an array. So, content_type_foobar could equivalently contain:

   'foreign keys' => array(
     'node' => 'nid',
     'node_revisions => array('nid', 'vid'),
   )

HOWEVER, there is more to it than this. For example, we need to be able to specify JOINS in the schema that are not representable as foreign keys. For example, we have tables that contain an "object id" (int) and a "object type" (string, e.g. "node"). The join is "object id = node.nid WHERE object type = 'node'". That can't be a foreign key because it is conditional, but we should still represent the relationship in the schema so we can automatically load nodes including this table's information. I haven't thought about the best way to do that yet.

Bottom line, don't hold up schema descriptions until we figure out how to represent foreign keys/joins.

webchick’s picture

Here's book.schema and comment.schema as well. I'm choking on cob-webs from comment module's schema. Hoo-boy.

I also made the changes Barry recommended: removed the 'foreign key' attribute, and instead documented foreign keys in the description with {table}.column.

webchick’s picture

...and contact/dblog for good measure.

8 files down, 15 to go:

modules/drupal/drupal.schema
modules/filter/filter.schema
modules/forum/forum.schema
modules/locale/locale.schema
modules/menu/menu.schema
modules/node/node.schema
modules/openid/openid.schema
modules/poll/poll.schema
modules/profile/profile.schema
modules/search/search.schema
modules/statistics/statistics.schema
modules/system/system.schema
modules/taxonomy/taxonomy.schema
modules/update/update.schema
modules/upload/upload.schema
modules/user/user.schema

If someone wanted to start at the end and work their way up, and someone else start in the middle somewhere and work their way up/down, this would get done a heck of a lot faster. ;)

webchick’s picture

Here's drupal and filter modules.

webchick’s picture

Nice! It turns out locale.schema was documented already; just needed to move the comments into description attributes. Thanks, Gabor! :D

drumm’s picture

For the record, I retract my negative comments since this is actually schema module's responsibility. I have not reviewed schema module itself.

yched’s picture

Thanks for this webchick, I'll try to help.

I'd probably advocate amending the coding style, and keep the current formatting for schema arrays, with all the attributes for fields on one line. Lets you have a quick overview of what a field is, and is more inline with what you get with tools like phpMyAdmin or most other db backends I know of.
I find the 'one property per line' format more difficult to grasp in this case, and big tables like menu or node would span over 2 or 3 screens, which would be a pain IMO.

$schema['actions'] = array(
  'description' => t('Stores individual actions that may be applied in the system.'),
  'fields' => array(
    'aid' => array('type' => 'varchar', 'length' => 255, 'not null' => TRUE, 'default' => '0',
      'description' => t('Primary Key: Unique action ID.'),
    'type' => array('type' => 'varchar', 'length' => 32, 'not null' => TRUE, 'default' => '',
      'description' => t('The module from which the action originates; for example, node or comment.'),
    ...
  ),
  ...
);

What do you think ?

webchick’s picture

I formatted them on separate lines because the coding standards dictate that that's the way that core does arrays everywhere. The standard makes both for increased legibility (though I could maybe see your argument here) and also so that diffs make sense (if you only change a not null to a true/false, it only affects one property, as opposed to the entire field definition).

If someone feels strongly enough about this that they want to go and re-roll the existing patch so that the properties match your sample, I guess I can go with that. Otherwise, I'll probably keep doing what I'm doing, since it is more consistent with the rest of core.

webchick’s picture

The block cache patch added another column to the blocks table which broke this patch. Re-rolled.

add1sun’s picture

FYI, I am starting at the bottom and moving up. I'm on user which will probably take me a wicked long time anyway, but in case anyone was looking to jump in, just skip user.

RobRoy’s picture

Minor thing...for descriptions we should not use a period when it's not a complete sentence I think, so we need to just be consistent with that so it doesn't look weird later on down the road.

webchick’s picture

Well, our comment convention is to have them all look like "Complete sentence." for consistency. I figured the same should apply to database comments.

webchick’s picture

I was dreading menu module because I hadn't been keeping up with the schema changes there, but turns out it's only one very simple table. Guess the complicated stuff's in system.schema. ;)

Current status:

Done
actions
block
book
comment
contact
dblog
drupal
filter
locale
menu

In progress
aggregator - need some help from some 0ld sk00l devs to explain some of those columns.
user - Addi's working on it.

Need volunteers
node
openid
poll
profile
search
statistics
system
taxonomy
update
upload

add1sun’s picture

Updated patch fixing changes to actions, locale and menu as well as adding what I have for user.schema. Got most of it but missing a few still marked TODO.

hass’s picture

subscribe

add1sun’s picture

Rerolled for a small change in HEAD and I've added upload and statistics. I've done the formatting for taxonomy and search but haven't got to filling the descriptions yet. That'll be in my next patch.

Also note that webchick and I decided it is best to do system last since that is most likely to change.

Current status:

Done
actions
block
book
comment
contact
dblog
drupal
filter
locale
menu
statistics
upload
user - still has a few TODOs in there

In progress
aggregator - need some help from some 0ld sk00l devs to explain some of those columns.
taxonomy and search - Addi's working on it.

Need volunteers
node
openid
poll
profile
search

system (hold off on this til the end)

bjaspan’s picture

I did the poll module. I also fixed several syntax errors; it is obvious no one has actually tried to *use* this patch yet. :-)

To use it, apply the patch and install schema.module. Visit admin/build/schema/describe and look at all the pretty tables. Your browser will take several seconds to render the page if you have JavaScript enabled; thank D6's automatic floating table headers (the page has a whole lot of tables...).

I suggest that we mark this RTBC even though it isn't done. Some schema documentation is better than none and, once committed, it can go back to CNW for more tables to be completed. Thoughts?

bjaspan’s picture

Priority: Critical » Normal

I do not see how this patch constitutes critical, however.

pwolanin’s picture

the description for {book}.bid is not accurate.

see attached.

webchick’s picture

I kind of would rather us have all documentation in place than only partial, but I'm not going to raise a huge fuss about it.

However, if we're going to commit this, we need to resolve those TODOs first. Any of you 'old school' Drupal folks want to lend a hand with those Aggregator and Search tables?

bjaspan’s picture

I am almost done with node.schema.

bjaspan’s picture

Latest patch includes node.schema EXCEPT for the node_type.modified field. I can find nothing in the documentation and almost nothing in the code about this field. Anyone?

Here's our current list:

Done
block
book
comment
contact
dblog
drupal
filter
locale
menu
node - has one TODO, node_type.modified
poll
statistics
trigger
upload
user - still has a few TODOs in there

In progress
aggregator - need some help from some 0ld sk00l devs to explain some of those columns.
taxonomy and search - Addi's working on it.

Need volunteers
openid
profile
search
system (hold off on this til the end)

Frando’s picture

you forgot the patch ;)

bjaspan’s picture

Dang!

I need to write an Attach Reminder module for Drupal. :-)

bjaspan’s picture

The schema for the boxes table changed; this patch tracks that change.

bjaspan’s picture

Resolved TODOs for node_type and users table.

JirkaRybka’s picture

Just did a small edit to the previous patch, adding a missing description on {locales_source}.version

I'm hardly the right person to document other core tables, but this field is my own recent addition, so I know it's purpose pretty well.

bjaspan’s picture

I think the core schema has stabilized enough to start working on system.schema; if is changes, we can track it. The new patch has TODOs for all tables except for the ones I've already done:

variables, history, system, url_alias

This leaves:

actions, actions_aid
batch
cache
files
flood
menu_router, menu_links
sessions

moshe weitzman’s picture

I'll pitch in with sessions table

TABLE: Drupal's session handlers read and write into the sessions table. Each record represents a "user", either anonymous or authenticated.
uid: 'description' => t('The {users}.uid corresponding to a session. Is 0 for anonymous user.'),
sid: 'description' => t('Primary key: A session ID. The value is geenrated by PHP's Session API.'),
hostname: 'description' => t('The IP address from that last used this session ID (sid).'),
timestamp: 'description' => t('The unix time when this session last requested a page. Old records are by PHP automatically.'),
cache: 'description' => t('The time of this user's last post. Is used when site has specified a minimum_cache_lifetime. See cache_get()'),
session: 'description' => t('an array of semi-permanent name-value pairs that belong to this session. Here Drupal persists the contents of $_SESSION for each user, including anonymous users')

bjaspan’s picture

Integrated sessions info from Moshe. Thanks!

chx’s picture

menu_custom
TABLE: Each entry represents a menu.

menu_name: t(The machine readable name of the menu.)
title: t(The human readable title of the menu.)
description: T(A description of the menu.)

menu_router
TABLE: Maps paths to various callbacks (access, page and title)

path: t('The Drupal path this entry describes')
load_functions: t('A serialized array of load functions (like node_load) to be called to replace a part of the currently visited path with an object.')
to_arg_functions: t('A serialized array of functions (like user_current_to_arg) to be called to create a string out of a part of the router path.')
access_callback: t('The callback which determines the access to this router path. Defaults to user_access.')
access_arguments: t('A serialized array of arguments for the access callback.')
page_callback: t('The callback that renders the page.)
page_arguments: t('A serialized array of arguments for the page callback.')
fit: t('A numeric representation of how specific the path is.')
number_parts: t('Number of parts in this router path.')
tab_parent: t('Router path of the parent tab.')
tab_root: t('Router path of the non-tab menu element this tab belongs to.')
title: t('The title for the current page, the menu item if exists and the tab if exists.')
title_callback: t('A function which will create the title. Defaults to t().')
title_arguments: t('A serialized array of arguments for the title callback. if the title_callback and these are both specified then title is ignored.'
type: t('Type of the menu item, like MENU_LOCAL_TASK.')
block_callback: t('Name of a function used to render the block on the system administration page for this item.)
description: t('A description of this item.')
position: t('The position of the block (left or right) on the system administration page for this item.)
weight: t('Weight of the element. Lighter weights are higher up, heavier weights go down.')
file: t('The file to include for this element, usually the page callback function lives in this file.')

menu_links forthcoming later today.

pwolanin’s picture

ok, edited some of chx's text for {menu_router} and added them into the system.schema file.

note - menu.schema already has description fields.

yched’s picture

I'll add batch table description ASAP :-)

add1sun’s picture

Updated patch with forum and taxonomy tables done. I also formatted the remaining schema files so all that needs to be done is TODOs filled in. Here is the list of .schema files that still have TODOs in them:
Aggregator
OpenID
Profile
Search
System
User

I'll pick up where I left off (about to begin Profile) and run it through schema.module later today.

hass’s picture

I think this is not good practice (only one example):
t('Drupal\'s session handlers read and write into the sessions table. Each record represents a user session, either anonymous or authenticated.')

...and this how it should look like - as i know:
t("Drupal's session handlers read and write into the sessions table. Each record represents a user session, either anonymous or authenticated.")

add1sun’s picture

Yep you are right about that. Latest patch has Profile tables done and cleaned up single quote strings.

hass’s picture

Maybe a stupid question, but why are you using "t()"? While i'm in setup (new install) i'm unable to select a different language in the early setup setp's or better to say my German translation gets imported in the last setup step only. This causes the strings to be written in English to the DB first, but later if i upgrade - and the language file is already in my install some fields become German - what ends with a mix of English and an additional language (maybe German) in the DB description fields. This sounds not good... It's good to have this translated - but should we end with a mix of different languages?

webchick’s picture

These strings aren't written to the database. They're for displaying in schema.module.

hass’s picture

Ups... ok :-). Why is this not written to the DB description fields, too? MySQL, MsSQL and so on... all have description fields and this is really helpful for DB development... i would love to not depend on a module if my MySQL Admin / Query is open

add1sun’s picture

Still rolling. This patch includes openID, actions and files. Here is a detailed list of the remaining TODOs:

FIELDS:
aggregator_category.block
aggregator_feed.image
aggregator_feed.etag
aggregator_feed.block
aggregator_item.guid
users.data

All of search.schema

IN SYSTEM.SCHEMA:
batch - table
cache - table
flood - table
menu_links - table
files.status - field
system.owner - field

webchick’s picture

@haas: because that would be a new feature, and is out of scope for this patch. Let's please keep this on-task, which is making sure the descriptions exist. We can figure out other interesting things to do with these descriptions later.

yched’s picture

Here's the description for system's batch table. Feel free to comment on wording or verbosity level if needed :-)

batch.bid : Primary Key: Unique batch ID.
batch.token : A string token generated against the current user's id and the batch id, used to ensure that only the user who submitted the batch can effectively access it.
batch.timestamp : A Unix timestamp indicating when this batch was submitted for processing. Stale batches are purged at cron time.
batch.batch : A serialized array containing the processing data for the batch.

yched’s picture

Small oops after double checking :
batch.token : A string token generated against the current user's session id and the batch id, used to ensure that only the user who submitted the batch can effectively access it.
(current user's *session* id)

yched’s picture

Er, I guess I might as well provide the patch.

Adds batch table.

Still TODO :

FIELDS:
aggregator_category.block
aggregator_feed.image
aggregator_feed.etag
aggregator_feed.block
aggregator_item.guid
users.data

All of search.schema

IN SYSTEM.SCHEMA:
cache - table
flood - table
menu_links - table
files.status - field
system.owner - field

moshe weitzman@drupal.org’s picture

Here is users.data description. I encourage the editor to remove profanity:

This pigpen shitpile of a field stores a serialized array of name value pairs that are related to a given user. Any form values posted during user edit are "helpfully" stored here automatically by user.module. See user_save() for more details. Further, Drupal loads all of these pairs into the $user object during user_load(). Use of this field is discouraged and it will likely disappear in a future version of Drupal.

add1sun’s picture

Rerolled for HEAD, fixed an apostrophe issue and added (a cleaned up version of ;)) Moshe's users.data description.

add1sun’s picture

Well, I'm pretty much running out of ones I can tackle. I made an attempt at search but we still have quite a list of TODOs:

FIELDS:
aggregator_category.block
aggregator_feed.image
aggregator_feed.etag
aggregator_feed.block
aggregator_item.guid
search_index.fromsid
search_index.fromtype

IN SYSTEM.SCHEMA:
cache - table
flood - table
menu_links - table
files.status - field
system.owner - field

msameer’s picture

aggregator_feed.etag = The etag HTTP header
aggregator_item.guid = guid RSS element

msameer’s picture

aggregator_feed.image = this has the HTML code that will show the image belonging to the feed (duh!)

Gerhard Killesreiter’s picture

cache is the generic Drupal cache table. Contributed modules can use it to store cached items, too.

flood is the Drupal flood protection table. It is used to count the number of flooding events per user. Examples: contact emails.

recidive’s picture

There are some tabs in node_schema() indentation.

pwolanin’s picture

Ha - my chance to beat chx to {menu_links} since he's probably asleep. Actually just noticed a minor bug in {menu_links} - it would be safer to have the default value for 'module' be 'menu' rather than 'system'. This is already being enforced in menu.inc in function menu_link_save(). Bad/weird things may happen if links are inappropriately seen as being owned by the system module.

see attached patch.

chx’s picture

Assuming I am asleep just because it's dead night is not a safe assumption :)

  • aggergator_category.block: Number of news items in block
  • aggregator_feed.image: an image representing the feed
  • aggregator_feed.etag a good explanation is at http://www.kbcafe.com/rss/rssfeedstate.html#entitytags
  • aggregator_feed.block: Same as for category, just this defines the block of a feed.
  • aggregator_item.guid : unique identifier for an item.
  • fromsid, fromtype: there is a node link tracker in search and this tracks the search item the link originates from. The fromsid is the object identifier and the fromtype is the type. So if node 123 links to 456 then fromsid is 123 and fromtype is node (and sid is 456 and type is node).
chx’s picture

  • flood table is used to control the number of events, like contacting someone. event is the name of the event (for eg. contact) hostname is the hostname of the visitor and timestamp is the timestamp of the event. By sotring these in the flood it's easy to count how many users your hostname contacted in the last hour. As all hostname based mechanisms, this is flawed as well, but binding to a session would be even more trouble -- too easy to circumvent.
  • files.status http://api.drupal.org/api/constant/FILE_STATUS_TEMPORARY/6
  • system.owner set for themes. It can be a .theme or a .engine .
webchick’s picture

Status: Needs work » Needs review
FileSize
155.68 KB

Ok. Added in remaining items, plus made a stab at a couple more that no one's mentioned yet, and here is a patch that I think is ready for review.

When reviewing, I'd much rather people check for things like:

a) Totally wrong information.
b) Typos or misspellings.
c) Syntax errors preventing modules from installing.
d) Anything else that would qualify as a "critical" bug.

Let's try and not let this patch get caught up in hashing over minor wording changes and that kind of thing. Remember that at the moment, Drupal has zero database documentation. If this gets committed, for the first time ever, it will have some database documentation! ;) And there will be plenty of time to opportunity the wording "just so" in the coming weeks/months. Let's get this sucker in!!

Btw, HUGE thanks to add1sun for taking this patch the rest of the way, and everyone else who provided input.

JirkaRybka’s picture

Component: database system » documentation
Status: Needs review » Needs work
FileSize
155.71 KB

My first review-pass (just reading patch file):

- Fixed double spacing between sentences:
{menu_links}.p1
{menu_links}.depth
{node_access}.realm
{variable}
{menu_router}.title_arguments
{system}.schema_version
{system}.weight
{sessions}.session
{url_alias}.language

- Corrected information:
{comments}.timestamp - actually this is LAST UPDATED time, but applies only to non-admin edits. This is weird and disrupts flat forums badly (http://drupal.org/node/55277), but for now it's how the filed works. Corrected.
{users}.pass - This is a hash obviously, not actual password. Corrected.

- Typos corrected:
{node_type}.orig_type - Broken wording ('The' vs. 'This') plus double space.
{system}.owner - Mixed single/double quotes, would probably result in parse error. Fixed.

- Still TODO - discovered, not fixed yet!
{search} - Description has TODO.
{cache_form}, {cache_page}, {cache_menu} - these are cloned from generic {cache} definition, inheriting it's description. That's wrong. Separate table-descriptions should be provided!
{menu_links}.external - description seems broken, or at least is unclear to me. Please fix.

JirkaRybka’s picture

Review pass two - functionality:

Installed recent 6.x-dev tarball with this patch applied, enabled all core modules. No problems, no errors/warnings. So code-wise it's OK for me.

add1sun’s picture

Just a note that this patch is now totally b0rked because of http://drupal.org/node/150245, where the .schema files all got moved into .install. I will try to get this rerolled today but it is a good sized project.

add1sun’s picture

Status: Needs work » Needs review
FileSize
154.57 KB

OK, here is a re-rolled patch with the schema hooks in the install files. I also cleaned up a few spacing/typo kinds of things and some of JirkaRybka's TODOs. The one TODO not tackled is the cache tables since I'm not sure how to approach that other than not reusing the cache table and actually typing each one out. I'll inquire and see. That said, I don't think that is a reason to hold this patch up.

Please review the patch text to see if we have any out and out misinformation or typos and check to see that there are no syntax errors that b0rk install. Setting back to CNR.

JirkaRybka’s picture

Currently I seem unable to apply patch for testing, or re-roll, due to current 6.x-dev tarball waiting for commited fixes. (No CVS tools on my system currently, sorry). So I'll comment without attachment this time:

I confirm the other two TODO's fixed, leaving only cache tables. In my opinion the single fields are generic enough to be OK for all cache tables, so IMO the fix may look something like this:

   $schema['cache'] = array(
    'description' => t('Generic cache table for caching things not separated out into their own tables. Contributed modules may also use this to store cached items.'),
    'fields' => array(
      'cid' => array( ...... the definition here ......
    );
 
   $schema['cache_form'] = $schema['cache'];
+  $schema['cache_form']['description'] = t('The description for cache_form table here');
   $schema['cache_page'] = $schema['cache'];
+  $schema['cache_page']['description'] = t('The description for cache_page table here');
   $schema['cache_menu'] = $schema['cache'];
+  $schema['cache_menu']['description'] = t('The description for cache_menu table here');

But I don't consider myself experienced enough to provide correct descriptions.

JirkaRybka’s picture

Status: Needs review » Needs work

Which means this is CNW.

I'm sorry for having no time to do this myself now.

JirkaRybka’s picture

Status: Needs work » Needs review
FileSize
154.66 KB

OK, I found some time to take this a bit further...

Attaching a re-rolled patch, with the cache tables done as I proposed above. I found three more instances of such cloned tables, so newly included are now these:
cache_form
cache_page
cache_menu
cache_update
cache_filter
cache_block
Please someone check the descriptions - I did my best based on both my experience and brief inspection of related code, but I'm far from being sure about the information being fully OK.

Note that this patch only just changes the table-descriptions for cloned cache tables, leaving field-descriptions inherited from generic cache table. I believe this is correct, because the field-level structure is identical for all cache tables, as well as the code handling it, so generic field-descriptions explaining the function in cache system itself are correct IMO. Only the basic purpose of whole tables is different, as reflected in this patch.

I checked all .install files - there are no more leftovers such as these cloned tables. No more TODOs.

I also fixed a few TABs in node.install (indentation).

I tested code by installing 6.x-dev with this patch applied and enabling all core modules. No errors, no warnings.

As a sidenote, Drupal module is about to being moved to contrib world ( http://drupal.org/node/178768 ). So we need to hurry with this version, or else re-roll after that happens.

JirkaRybka’s picture

Title: Document database schema in .schema files » Document database schema in hook_schema()

Oh, and the title also needed a little fix - we have no more .schema files. :)

add1sun’s picture

The removal of drupal module has happened (http://drupal.org/cvs?commit=83605) so I've re-rolled the patch to keep current with HEAD.

add1sun’s picture

If folks could review and/or get this into HEAD before http://drupal.org/node/181955 gains any traction, that would be great, so we don't have to do a non-substantive reroll yet again.

bjaspan’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
155.54 KB

The menu_custom table's description was included in the 'fields' array instead of the top-level table structure. I moved it. New patch attached, now RTBC.

FYI, I enhanced the dev version of schema module to report missing table or field descriptions as errors on the Describe page. That's how I found the menu_custom error. I figure this will also be a good way to motivate contrib authors to add descriptions to their tables.

Gábor Hojtsy’s picture

I looked through the beginning more closely, and then cherry picked some descriptions. All I checked seemed to look good. The question before I commit is whether it was checked that the resulting schema is the same, ie. was there any mistake while reformatting and copy-pasting stuff?

sun’s picture

http://drupal.org/node/181955 is waiting for this to be committed.

bjaspan’s picture

#72: Damn! Good catch, Gábor. As the person who keeps harping on the fact that we need to test for schema consistency, I'm ashamed that I missed this.

Yes, the patch introduced two schema errors, one in menu_links and one in term_node. I have now fixed both. Testing method: Wipe database, clean install without patch installed, install patch, activate Schema module, perform the Comparison test. Post-fix, schema reports no errors on either mysql or pgsql.

New patch attached.

JirkaRybka’s picture

bjaspan: Sorry, but both of your last 2 patches didn't apply for me. All hunks failed, because you rolled the patches with Windows-style CR/LF line endings. (The option --strip-trailing-cr might help next time, or not, but is worth a try.)

As I was unable to force my patch utility to accept that, I took #69 patch and re-added the menu_custom fix as described in text. Then I tested it, and found exactly the same as you did - menu_links and term_node. Fixed also that.

So we finally both reached exactly the same state I believe. I'm attaching my patch only because your one didn't apply due to CR/LF again, otherwise they should be identical.

I tested the attached patch using this method:
- Installed two instances of 6.x-dev (with and without patch), with all core modules enabled, using exactly the same steps and settings for both.
- Exported both databases via PhpMyAdmin (as sql dump, only structure, no data).
- Did a diff on these two dumps. The only difference was database name and timestamp.

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Fixed

Hm, I diffed your diffs to see what is the difference, and indeed apart from newline differences (which I omitted with diff -ub), there were nearly only file diff header differences. BUT there was also a little actual difference, which looked interesting. It was around the index definitions in taxonomy_schema. But closer inspection of the diffs showed that the difference is actually because of one diff not modifying a line while the other swapped two lines for some reason.

That said, I committed bjaspan's patch. Thanks everyone for your efforts to make Drupal's database schema as documented as it never was before :)

Anonymous’s picture

Status: Fixed » Closed (fixed)
gpk’s picture

For a diagram for D6 see http://drupal.org/node/184586.

lalit774’s picture

Title: Document database schema in hook_schema() » multiple database using hook_schema?
Version: 6.x-dev » 7.x-dev
Component: documentation » database system
Priority: Normal » Major
Status: Closed (fixed) » Active
Crell’s picture

Title: multiple database using hook_schema? » Document database schema in hook_schema()
Version: 7.x-dev » 6.x-dev
Component: database system » documentation
Priority: Major » Normal
Status: Active » Closed (fixed)

It is rude to re-open a long-dead issue and change its focus without so much as an explanation. If you have a new bug or feature request, please open a new ticket.