Schema API 1

subscribing

Nice writeup, Barry.

Why do we want all this?

• Supporting more database engines, be it oracle, sqlite or mssql, becomes a lot easier.
• By now, only a minority of our contributed modules really support postgres, because of missing CREATE TABLE statements for pgsql. No more with this patch - we basically get postgres support for all of D6 contrib for free.
• update and install hooks are much simpler now
• there are many many more potential features that can be built upon this patch - Just to name a few, automatic database updates, a simple and consistent CRUD API, form scaffolding, simpler cck & views, schema-based data validation, etc. come to my mind (note: nothing of this is intended for drupal 6). For all this, this patch is a door opener.

I just wanted to note that there is already quite a lot of thought and work in this patch - it has already seen 15 revisions, and is based on similar concepts written by bjaspan, dopry, and myself. This patch is the much reworked combined effort to merge, test, improve and extend these concepts. Interested parties can skim http://drupal.org/node/136171 for more details.

bjaspan : the 'drupal requirements' page http://drupal.org/requirements states we support pgsql 7.3+
Do you think there can be incompatibilities ?

For development, I use Apache 2, PHP 5.1.2, Postgres 8.2 and MySQL 5.0 on Linux and PHP 5.2, MySQL 5.1 on Windows for additional testing.
I don't think that there will be any problems on PHP4 or MySQL 4, though. Not sure about Postgres 7.3.

Not to open a can of worms, while various people have expressed a desire for specific versions of MySQL and we are moving towards that, no one has had an equivalent discussion or research for PostgreSQL minimum version.

Just looking at their website releases they have as current download releases are 8.2.4, 8.1.9, 8.0.13, 7.4.17

What version should we continue to provide support for? Act now those who are interested.

subscribe ... this is some kick add engineering, along with lots of perseverance. i will test soon. thanks all.

Subscribing.

The first thing I notice (thanks to dmitri pointing this out) is that in install.php, comment and taxonomy's schemas (which are optional modules) are at the same 'level' as block, filter, and other required modules. We discussed on IRC, and this is because traditionally these tables were added in system.install. However, since we're breaking out all the tables to their respective 'spheres of influence' (ex: users, user_roles, etc. going into 'user.schema'), it makes sense to also separate these out into their own .install files. It bloats the patch a bit, but not too badly, and fixes a long-standing inconsistency.

Then I looked at what .schema files were handling what tables. The only inconsistencies I noted were:
* authmap belongs with user.schema
* I thought url_alias belonged in path.schema, but Eaton pointed out that this is referenced in core (path.inc) with or without path.module, so that can stay in system.schema.
* I would move the menu* and cache_menu tables to menu.schema.. although I think the goal is to make menu.module not required, so menu.schema should be referenced at install.php, it's just a nice level of separation and makes it easy to see in one place what tables correspond to menus for new developers.

I LOVE the fact that you can just do:

 drupal_uninstall_schema('aggregator');
 

instead of:

  db_query('DROP TABLE {aggregator_category}');
  db_query('DROP TABLE {aggregator_category_feed}');
  db_query('DROP TABLE {aggregator_category_item}');
  db_query('DROP TABLE {aggregator_feed}');
  db_query('DROP TABLE {aggregator_item}');

We discussed whether or not it made sense for the uninstall system (and install system for that matter!) to handle this itself; this was discussed in the previous patch and was decided not to do this kind of 'magic', at least at this stage. I do think it would be a nice API change, to only have to declare hook_install and hook_uninstall if your module's doing something special like setting the module weight at install time, or deleting unwanted variables. Feel free to call this 'scope creep' at this point, but we're running out of time to make API changes, and it'd be great if we could get this in for this release. :)

Haven't done a functionality run-through yet (will try and do that later today), but that is my 'code' review.

Unfortunately, we can't just make creating comment.module's schema optional (e.g. creating it when comment.module gets activated), as it's referenced even if comment.module is not activated. Similar with taxonomy.module, I think. I'd call this a bug, but let's leave that out for a follow-up patch, please.

Anyway, here's another reroll.

Changes:

• Sync with HEAD
• moved authmap to user.schema
• Added a function that returns the unprocessed and unaltered version of a schema. This is needed to be able to actually inherit existing table definitions. This made it possible to use cache_filter to filter.schema and cache_menu to menu.schema
• Documenation improvements. Added PHPDOC @defgroup schemaapi and @ingroup statements for all schema api funciton

subscribing

Subscribing, since I need to help figure out the menu upgrade path.

for the record, we don't try to support HEAD -> HEAD updates. therefore, i'd be all in favor of porting all of the existing D6 updates to the schema API once this lands (as a follow-up patch, no need to hold this up!). it'd be great to have a nice body of example updates sitting in core for contrib authors to use as real life "best-practice" guidelines when trying to write their own updates. so, there's no need for this "menu's update will be the first to use schema" thing. ;) granted, it might be the first *actually* written to use the schema API, but it shouldn't be the first numerically, speaking...

still haven't had a chance to look at the code or test it, but i hope to do so Real Soon Now(tm)...

thanks again to bjaspan, frando, nedjo, et al for this amazing effort!

It would be great to see some sort of automatic update system to identify necessary changes to acheive the current, specified schema for a module. It shouldn't be terribly hard to compare columns, tables, and keys to identify what needs to DROP, what needs to be CREATEd, and what needs to be ALTERed. If a module's updates exceed the capabilities of automatic detection, then the fallback can be a manual update. There could also be validation that the final, updated schema matches the given one.

subscribing... because I'll use it here
http://drupal.org/node/122241

It could be really nice to add human readable descriptions to the array returned by hook_schema. This way, it would be easy to print a helpful schema of the whole drupal database :

function system_schema() {
  $schema['authmap'] = array(
    'help' => 'Description of the authmap table, what it does, what is it\'s role inside Drupal',
    'fields' => array(
      'aid'      => array('type' => 'serial', 'disp_width' => 10, 'unsigned' => TRUE, 'not null' => TRUE, 'help' => 'description of this field'),
      'uid'      => array('type' => 'int', 'not null' => TRUE, 'default' => 0, 'help' => 'description of this field'),
      'authname' => array('type' => 'varchar', 'length' => 128, 'not null' => TRUE, 'default' => '', 'help' => 'description of this field'),
      'module'   => array('type' => 'varchar', 'length' => 128, 'not null' => TRUE, 'default' => '', 'help' => 'description of this field')
    ),
    'unique keys' => array('authname' => array('authname')),
    'primary key' => array('aid'),
  );

[...]

A db_schema_sumary module (inside devel?) would call all those hook_schema() functions and would pretty print a DB schema. If documentation can be put near data definition, there is hope authors will add those descriptions :-)

Both field/table description and automatic database updates are nice additions to this patch. These are both additional features, though, so if you are interested in them, please please review this patch and help to get it commited before the freeze. Then, open follow-up issues for these features. Thanks.

I'd at least like to see a minor change: support for foreign keys. I know Drupal 6 core won't use it because it still needs to be MyISAM-compatible, but plenty of contrib modules with less broad compatibility might use it.

sub

There are lots of small things that need to be fleshed out: drupal vs Drupal, documentation being rather technical, code style, TODOs in the code, etc. It's too much work to point out all of these small things. I suggest to run things through the code checker.

Function names like drupal_process_schema() are not very descriptive, and the PHPdoc isn't particularly user-friendly either (i.e. @param $module The module whose hook_schema returned $schema.). I think we'll want to flesh out some of the PHPdoc, and try to make both the API and the PHPdoc more accessible to developers that are not DB experts.

Also, what is 'disp_width'? I prefer to write 'display_width'.

That said, I'm really interested in this patch. Not so much in the abstraction it provides but in what this patch will enable in future.

I also care about things like referential integrity and would like to see Drupal support this better. I'd like to understand how hard it is to implement it on top of this patch. If this patch makes it harder, than that is a bad thing. So I think the question about referential integrity was appropriate. I talked to core PostgreSQL contributors and they care deeply about referential integrity. For them, referential integrity is key in order to embrace Drupal. I understand that implementing this might be outside the scope of this patch, but it is still a valid question and I'd like to have a feel about some of these things before I feel comfortable with this patch.

All in all, this looks like a big step in the right direction. At the same time, it feels like this patch needs a little bit more massaging to get at least the details (coding style, API and PHPdoc) up to core quality.

PS: isn't the plural of 'index', 'indices' instead of 'indexes'?

Another question I have: how do we know that this will map well on MSSQL, Oracle, etc? If I look at RoR, their schema definition seems to be slightly more high-level.

    create_table :accounts do |table|
      table.column :name, :string
      table.column :budget, :float
    end

I should probably read up on this in the schema group. ;-)

Thanks for your review, Dries.

I agree with everything bjaspan said. I think this is patch is a huge step forward towards being able to have referential integrity, and it allows us to notate such references in a reusable way.

I also do think that our schema definitions are abstract enough. I guess all databases that claim to understand SQL have some form of constraint for allowing or disallowing null values and for specifying default values. And they have different data types, which can be mapped to our type/size combinations (if a db engine would e.g. have only one data type for integers, it would just map this type to all sizes). Engines don't necessarily have to support all possible attributes.

Rerolled patch attached.
No API/functionality changes, but synced with HEAD, incorporated recent schema changes, corrected all code style issues that were found by either me or code-style.pl, improved documentation here and there, made db_process_schema internal as that's what it is.

I gave this patch the 'webchick' treatment in terms of functionality tests, and it appears to be solid... tested creating nodes (forum, book and poll included), taxonomy, path aliases, creating some profile fields, uninstalling modules, running update.php (from a clean install, granted). All appears solid.

Working on a 'code-level' review of the patch now.

the only thing that i've come across would be adjusting .htaccess file to add *.schema to the list of protected files.

* At first glance, the difference between drupal_get_schema() and drupal_load_schema() was not obvious. Here is how we might be able to improve this: drupal_load_schema() should probably be drupal_load_schemas() -- we seem to be loading multiple schemas, not just one schema. The other Drupal core components often use 'init' to initialiaze something. drupal_init_schemas() might be a better name if that is what it does. I leave that up to you to decide. Alternatively, you could merge both and initialize the schemas when drupal_get_schema() is first called.

* _drupal_process_schema: the word "process" doesn't mean much. It's like calling a variable $variable. There might be a better name for this function that is a little bit more explanatory.

* "Returns the unprocessed and unaltered version of a table's schema." Please clarify what an "unprocessed table schema" is, and why it matters? Explain when you want to use a processed and when you want to use an unprocessed table schema. Don't assume that aspiring developers will automagically figure this out. Be more verbose about things like this.

* The PHPdoc comments are not consistent. Somethimes you write "@param foo \n explanation", sometimes you write "@param foo explanation" (no line break).

* Coding style: "} else {" should be on two lines.

* Some code comments end with a dot, others don't have a dot, yet others end with ':'.

* $sql = substr($sql, 0, -3) . "\n) "; looks odd ... maybe add a code comment. There are a couple such statements.

* We never use 'DATETIME' -- not sure whether it should be supported. Probably doesn't hurt.

I found some minor issues with the PHPdoc comments and coding standards conformance, and told frando about those in "real-time" ... nothing like real-time nit-picking. ;)

Once those are dealt with, I really don't have any further complaints. Great work!

Lovely patch. I only found minor issues.

• _drupal_process_schema should be named _drupal_schema_add_defaults() and _drupal_schema_add_defaults_field()
• require_once is slightly faster and more standard for us now that include_once. use require_once if possible for the mysql-common.inc
• the latest patch i tested has a change for favicon handling in theme.inc
• personally, i could do without comments like '+ // Create tables' and '+ // Remove tables' in every .install file

Thanks for the reviews!

I hope that I fixed all issues that came up (some more on IRC).

So,
I corrected the remaining code style stuff that I found and added more comments. For a general introduction to hook_schema(), we might also want to create a handbook page. I merged drupal_load_schema into drupal_get_schema, and renamed _drupal_process_schema to _drupal_initialize_schema. require_once instead of include_once. 'display_width' is no more (we don't need it).

Minor changes: fixed about 15 spacing errors and another 60 changes for typos, string quotes, comment style, etc.

Doing a HEAD -> HEAD upgrade works except for giving an error on the mlid change, which is a serial type. Apparently it's not mapping to INT correctly?

ALTER TABLE {menu_links} CHANGE mlid `mlid` DEFAULT NULL

Also, in includes/database.pgsql.inc the code comment reads "Put :normal last so it gets preserved by array_flip." I couldn't find any actual use of array_flip in the database code though - is this still relevant? If it is there are a few places where :normal isn't placed last.

Patch applies cleanly, install and enabling all modules proceeds without error.

Here ya go!

i'd really love to RTBC this as is. however, a few things stop me:

1. the patch is now 4515 lines long, and i don't have time to carefully review all of that right now. :(
2. a fresh D6 install worked fine. however, when i dumped a clean D5 test site's DB, imported that into a new DB for a D6 test site, and ran update.php on that site with this patch installed, update 6004 failed because of a duplicate KEY:
   user warning: Duplicate key name 'created' query: update_sql ALTER TABLE users ADD KEY created (created) in /Users/wright/drupal/cvs/6/core/includes/database.mysql.inc on line 163.
3. for some reason, "menu_schema" is showing up via drupal_set_message() at the end of update.php

that said, i'd *love* to see this go in, ASAP... i'm just swamped today and can't do what it takes to drive this patch home right now. #2 and #3 could certainly be cleaned up/fixed in separate patches, since this API/feature change is too important to delay it over trivialities like that.

ok #3 is caused by a silly debugging line at the top of menu.schema. that can be trivially removed anytime, it'd just take me a lot of work to re-roll that based on having to add all the .schema files to the patch, so it'd be easier to do after this patch lands (or just remove the drupal_set_message('menu_schema'); from menu.schema before you actually commit this to core).

#2 is caused by a bug unrelated to this patch, and in fact, update 6004 hasn't been ported to schema API, anyway. if you install a clean D5 directly from HEAD, system_update_1022 is never run, but the {users} table has the created key already. therefore, the key is there, but the "system_update_1022" variable is never set. seems like (if this is really how we plan to solve this problem), we need to set that variable in system_install() in D5, to avoid update 6004 doing anything in this case. however, seems like it'd be better to clean all of this up via porting the D6 updates to schema API and knowing if the KEY is already on the table when we hit update 6004, anyway...

#1 is still a minor problem, but it's no reason not to commit this. it's gone through many iterations, reviews, etc. we've got plenty of time to tweak comments, improve docs, or fix any bugs that might crop up.

therefore, setting this back to RTBC with a clear conscience.

@bjaspan, thanks for #43. removes the only minorly ugly part of the previous RTBC.

@dries, gabor: #43 gets the dww-seal-of-RTBC'edness, please commit ASAP so we can start building on top of this.

thanks, all!

Tested without a problem. Since this patch touches everything I just tested about every function in drupal (where there is a db query) I could think of. No problems. This looks nice!!!!

my partner and i just submit a complete patch/package for oracle supporting in drupal-6.x-dev (http://drupal.org/node/136649#comment-249207), and so hope to have some more idea about the relationship with DDL supporting:

1. if we hope to add DDL supporting into our current work, what function/API need to add?
2. is DDL going to be drupal-6.x feature? or it is still under progress and will not go into coming on stable release (6.x)?
3. due to my understand, the relationship of DDL/DML/database.xxx.inc seems to be the following model. am i correct, or any miss? is DDL a must to database.xxxx.inc, or just a feature add to existing database API?
   

   |---------------------|
   |     yyy.install     |
   |---------------------|
   |     database.inc    |
   |---------------------|
   |    DDL   |    DML   |
   |---------------------|
   |  database.xxxx.inc  |
   |---------------------|
   |      database       |
   |  (mysql/pgsql/etc)  |
   |---------------------|

4. according to this message, is that means development of other database supporting is able to work out in parallel with DDL, and further more add DDL back when it is ready to merge into drupal core, as an official standard of drupal database API?
   

   The Schema API does not change the way that current install and update hooks function and track schema version numbers. However, once hook_schema() is defined, a typical module's .install file becomes a lot simpler:

i really hope to add oracle supporting into drupal-6.x, and also hope to take the benefit of DDL. hope can help in speeding up DDL supporting in coming on stable release, and so the work of my side can also greatly reduce ;-)

Drats. Patch no longer applies. Needs a quick re-roll!

ugh, all because of this 3 line commit - http://drupal.org/cvs?commit=68496
- Patch #140218 by drewish: fixed MySQL 5 strict bug.

Dries: perhaps you could undo that revision ( cvs update -j 1.116 -j 1.115 modules/system/system.install), then commit #43 from here, and tell Drewish to re-roll his 3 line patch, instead of having barry re-roll this 4500 line monster? ;)

Nothing hard about re-rolling it; in this case the easiest way is to just manually edit those two lines in the patch file.

hehe, yay. ;) thanks, Chris. #52 applies cleanly, and therefore, is indeed RTBC again...

Committed. We'll want to document this:

1. Mail to development mailing list.
2. Handbook page and upgrade instructions.

Marking as 'code needs work' until documentation is available.

Sorry - my fault for not thinking to update menu.schema also. Here is the one-liner to fix it.

Oops, one more time.

since menu module doesn't work right now (needs this patch first: http://drupal.org/node/145058, then: http://drupal.org/node/144753) this doesn't really matter.

Maybe I'm missing something but how do you rename a table with the Schema API?

I'm looking forward to having the upgrade instructions ;)

i'm not sure if this is a bug or a documentation issue

this bit of code which i'd thought would add a new column to the files table:

db_add_field($ret, 'files', 'status', array('type' => 'int', 'not null' => TRUE, 'default' => 0));

generates the following sql:

ALTER TABLE {files} ADD status `status` INT NOT NULL DEFAULT 0

which causes an error:

user warning: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'status` INT NOT NULL DEFAULT 0' at line 1 query: ALTER TABLE files ADD status `status` INT NOT NULL DEFAULT 0

perhaps the following is to blame:

function db_add_field(&$ret, $table, $field, $spec) {
  $query = 'ALTER TABLE {'. $table .'} ADD '. $field .' ';
  $query .= _db_create_field_sql($field, _db_process_field($spec));
  $ret[] = update_sql($query);
}

i'm guessing field is getting inserted twice here.

here's a patch to fix the issue in my last comment.

it's rolled relative to the includes directory rather than the root, i apologize but i've got a bunch of other changes and this cvs client won't let me select what files to include if i roll from the root.

Yep that looks good. As it turns out there is no use of db_add_column in the MySQL version of the core patch, so testing did not turn up the error. PostgreSQL does use db_add_column within its db_change_column implementation, but the db_add_column implementation for PostgreSQL is different and doesn't have that bug. Tricky :P

While looking through the code I also happened to notice that the comment for MySQL's db_change_field() ("Remember that changing a field definition involves adding a new field and dropping an old one...") appears to be inaccurate - the add/drop strategy is only used in the PostgreSQL version. Correct?

Committed the fixes. Setting back to code needs work until documentation is ready.

Ok, so I'll get stupidity credits for not noticing this already, but why are the tables for menu.inc in menu.schema?

Despite the name, these are system tables and need to be installed even if the (now optional) menu module is not installed.

looking at the files more- there doesn't seem to be the table for the menu module yet. Anyhow, I'm about to work to get the menu.inc patch re-rolled: http://drupal.org/node/145058

In this patch I'll have the menu_router and menu_links in system.schema and this subsequent issue will add the menu module table to menu.schema: http://drupal.org/node/144753

@bjaspan
{menu_router} is required to serve pages, so it's critically important.
{menu_links} is used to hold menu links- like the items in the Navigation menu, or Primary links
menu module will be essentially a UI to add to or manipulate these menu links, or to add custom menu blocks.

I am working on Oracle support for Drupal,
as the VARCHAR2 datatype has a limit of 4000,
in this case, CLOB datatype is a alternative.
however, LOB cant support much operation as VARCHAR2,
LOB cant work for some kind of queries,
such as ORDER BY and GROUP BY.

I have to decide rather VARCHAR2(4000) or CLOB is used case by case.
I need a variable/hint in API to determine the field is a large data field
or it is a long text with content comparison.
Could I have this feature in API?

i posted a patch to add a db_rename_table function: http://drupal.org/node/147285

'type': The generic datatype: 'varchar', 'int', 'serial', 'float', 'numeric', 'text', 'blob' or 'datetime'. The types map to the underlying database engine specific datatypes. Use 'serial' for auto incrementing fields.

'size': The data size: 'tiny', 'small', 'medium', 'normal', 'big'. This is a hint about the largest value the field will store and determines which of the database engine specific datatypes will be used (e.g. on MySQL, TINYINT vs. INT vs. BIGINT). 'normal', the default, selects the base type (e.g. on MySQL, INT, VARCHAR, BLOB, etc.).

» http://drupal.org/node/146939

it seems to be a great idea! BTW, for the case of oracle, the select of 'normal' mapping may not only related to supported size but also for maximum feature compatibility, as stay in #72. e.g. we may map in this way:

    'varchar:normal'  => 'VARCHAR2',

    'text:tiny'       => 'VARCHAR2',
    'text:small'      => 'VARCHAR2',
    'text:medium'     => 'VARCHAR2',
    'text:big'        => 'CLOB',
    'text:normal'     => 'VARCHAR2',

but this will also means need to be careful in choosing the most suitable size of data type during development. we may need some document about:

1. what size is suggested to used in what case
2. what feature are expected for that size
3. what size is it really mapped to under layer DB

or else developer will confuse about that, and code may not be able to work across different DB.

does schema API support for dynamic DB creation? it is always happened within cck and views module?

since oracle DB have a limitation of maximum 30 characters for table/column name, trigger, index and some other stuff. my team is planning to create a mapping table for it, e.g. mapping xxxxx into xxx_1, as like as MS Win95/DOS long filename handling. if both static DDL (run during module installation) and dynamic DDL (create table on-the-fly) will be handled by schema API, we will be able to catch all exceptional case and take action for such mapping. may i have some more information about this?

Thanks bjaspan. :-)