There is no reason for hook_schema() to be in a separate .schema file. It is closely related to the update functions which live in .install, so having it in .install is convenient. The schema is only collected when modules are enabled/disabled (or, eventually, when CCK types change), and then it is cached, so putting it in .install will not require the .install files to be parsed that often.

So, we should move hook_schema() into .install and remove the .schema files. Not a top priorit, though.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

webchick’s picture

Status: Active » Needs review
FileSize
114.06 KB

Ah, shoot. Didn't see this issue, so marked http://drupal.org/node/155220 as a duplicate.

Here's a patch.

bjaspan’s picture

Test methodology:

1. cvs update to HEAD, install into empty database, enable all core modules, mysqldump > pre-patch.sql.
2. Apply this patch, install into empty database, enable all core modules, mysqldump > post-patch.sql.
3. Run "diff -u -I '^INSERT' pre-patch.sql post-patch.sql", verify that the only difference reported is a timestamp.
4. Run "/usr/bin/find . -name '*.schema' -ls" and verify that all the .schema files are 0 bytes.

That all worked fine, so that part of the patch is RTBC.

However, drupal_get_schema(), drupal_install_schema() and related functions in common.inc assume that hook_schema() is implemented in a .schema file, not the .install file. So, I updated those functions to load .install instead of .schema, and now kick the new patch back to Angie for review. :-)

webchick’s picture

I think that last patch was missing a couple things (like node.install which was added, etc.) so I re-rolled my initial patch, adding Barry's changes, and also including actions.schema.

webchick’s picture

Status: Needs review » Needs work

Crap. That didn't work for some reason.

webchick’s picture

Status: Needs work » Needs review
FileSize
118.7 KB

Ok, there we go.

Tested clean install, and enabling all modules. Seems to work ok, but I'd love for someone else to mark it RTBC.

webchick’s picture

Status: Needs review » Closed (won't fix)

I showed this patch to Earl and he pointed out that for Views (and presumably any other module that deals heavily with the database layer, like import/export API, etc.) it would be helpful to keep the schema files separate so that it's not loading the entire .install file with all the update hooks, hook_requirements, etc. each time it does anything. So given that there's a good rationale for keeping these separate, I'm marking won't fix.

bjaspan’s picture

Status: Closed (won't fix) » Reviewed & tested by the community
FileSize
121.39 KB

I disagree with Earl. Yes, the advantage of having hook_schema() in .schema and not .install is that we don't have to load .install stuff when hook_schema() is invoked. However, hook_schema() is only ever invoked (a) when modules are installed or (b) when CCK data types change. In other words, "almost never." The results of hook_schema() are stored in the cache so any module that just uses the schema information gets it from there, not by invoking hook_schema(). Loading the .install files does not take *so* much time that we need to avoid it from happening this infrequently.

On the other hand, the advantages of having hook_schema() in .install files are (a) that is where every developer will expect them to be, (b) it eliminates Yet Another Small Per-Module File, and (c) it keeps all the database-schema related stuff in one place, making consistency more likely.

Admittedly, it is a minor point either way, but I think it is clear that the schema info "belongs" in .install and the disadvantage of having it there, namely the very slightly increased page-load overhead for the rare case of module/schema changes, is barely relevant and certainly outweighed by the advantages.

So, I have attached the patch re-rolled for HEAD and since webchick and I have both reviewed and approved it I'm marking it RTBC. I don't know how this patch relates to code freeze. This is a "code reorganization" patch. I'd call it a "code organization bug fix." It is not a new feature or an API change, though I guess you could say moving the required location for hook_schema() from one file to another is an API change.

If this doesn't go into D6, we'll have so many .schema files in contrib that we'll be stuck with them. That won't be the end of the world. If the core commiters change this to wontfix or postponed, so be it.

RobRoy’s picture

FWIW, I agree with bjaspan. hook_schema belongs in .install and the clarity far outweights the small overhead gain.

Dries’s picture

Priority: Normal » Critical
Status: Reviewed & tested by the community » Needs work

I'd still like to commit this but it needs a re-roll for locale.schema and node.schema.

webchick’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
118.05 KB

Here we go; this cures the failed hunks. Tested clean install + enable all modules and it seems to work.

rstamm’s picture

Status: Reviewed & tested by the community » Needs work

.schema files are protected from prying eyes now, http://drupal.org/cvs?commit=72331

Patch should remove it from .htaccess.

cburschka’s picture

Status: Needs work » Needs review
FileSize
105.63 KB

Sounds like a change that would be simple enough, but this is a massive patch. I checked out a clean HEAD and applied it, but even without changing anything I just couldn't get a new re-roll to match in size even closely - about 12kB are missing, and comparing the patches with diff again doesn't help either. There was fuzz in system.install, but I couldn't believe that alone would cause such a difference.

On the off chance that there is an obvious and harmless explanation for this discrepancy, I'm attaching my re-roll, but it would probably be safer if rerolled by webchick.

There appears to be something odd - CVS doesn't recognize these files:

? modules/block/block.install
? modules/filter/filter.install
? modules/node/node.install
? modules/taxonomy/taxonomy.install
? modules/user/user.install

They obviously are there, and running cvs update on them works, but running cvs diff triggers an "I know nothing about ..." message. I'm sure my version is clean and fresh from HEAD, though. Baffled.

cburschka’s picture

Status: Needs review » Needs work

Oops. Sorry. That was dumb. The files didn't exist, but the patch added them, and I neglected to roll my patch so new files would be included.

I don't know where I got the idea that the files were in CVS, but they are obviously not. I'll attach a proper patch in a moment.

cburschka’s picture

Status: Needs work » Needs review
FileSize
125 KB

Figuring out how to put new files into a patch took some doing, but we all have to learn. :) Thanks to chx for the help.

The attached patch is now a few kB larger than the old one, but my diffing adds the extra lines

RCS file: /cvs/drupal/drupal/*
retrieving revision *
diff -u -p -r* *

Which together with the .htaccess change can roughly account for the difference, so I think this one's okay.

rstamm’s picture

FileSize
124.59 KB

Patch didn't apply anymore, I've rerolled it.

ChrisKennedy’s picture

Status: Needs review » Needs work

Needs to be re-rolled.

moshe weitzman’s picture

Priority: Critical » Normal

We are working down the Criticals queue in Barcelona. Since Drupal works fine without this change, it isn't Critical.

webchick’s picture

This is actually probably a won't fix issue at this point... I'll leave it up to bjaspan.

bjaspan’s picture

I actually think this is important as demonstrated by the number of core patches that got committed with changes to .schema but not updates in .install. If a core committer gets it wrong, what chance do most contrib authors have of remembering? So I think fewer people will be confused if we put hook_schema in .install.

The patch is way out of date by now. I don't especially feel like taking on this issue right now, but if I do not we will be stuck with .schema files forever.

Anyone else want to opine?

cburschka’s picture

What needs to be done to get this patch up to date?

I would think the easiest way would be to start over, moving the .schema stuff into .install, since so many of them have been changed by now. But the patch apparently involves more than just moving the file contents, and I'm not quite sure what.

webchick’s picture

The only other changes are the first two hunks from http://drupal.org/files/issues/drupal-schema-to-install-150245-2.patch which have it looking in .install rather than .schema and fix some code documentation.

cburschka’s picture

And the .htaccess bit, I guess?

I've re-entered the first four hunks from patch #15 (http://drupal.org/files/issues/drupal-schema-to-install.txt), which leaves only the file contents. Needless to say, this copy-paste is unspeakably dull and repetitive. But I'm almost through...

cburschka’s picture

Status: Needs work » Needs review
FileSize
125.96 KB

Argh. Here it is. Based on file size and lack of error messages, it seems about right, though I haven't looked at the patch file in detail or tested it with a fresh install yet.

bjaspan’s picture

Status: Needs review » Reviewed & tested by the community

A tested this with mysql by making a clean installation, enabling all core modules, and dumping the database both with and without the patch installed. The dump files were identical except for timestamps meaning that the table structures were the same which means all the hook_schema()s did the right thing.

Since the only question is whether hook_schema() still returns the right data, and the mysql test proves it does, I do not see why a test with pgsql is needed, and since pgsql does not have a plain-text dump like mysql, I'm not sure how to do such a test anyway.

RTBC.

bjaspan’s picture

If this gets committed, I'll make an announcement in several places so that module authors will know to move their hook_schema() functions. I'll also update the handbook.

Dries’s picture

Status: Reviewed & tested by the community » Needs work

OK, I committed this patch. Thanks for all the work.

We'll want to update the documentation though. Therefore marking as 'code needs work'.

eaton’s picture

I may have missed something, but isn't the idea that other modules like views would have a much larger batch of information integrated into the .schema file? refrential integrity enforcement, etc etc...

I know I came in late on this patch, I think I just missed an important piece.

killes@www.drop.org’s picture

Is it possible that this patch broke new installs? Only a part of the tables gets installed. Missign are for example users, blocks, and access.

killes@www.drop.org’s picture

I think the patch is alright, but Dries forgot to "cvs add" the new install files...

dvessel’s picture

FileSize
37.57 KB

I have the same problem as killes.

Attached the errors spewing out. This happened after submitting the db name and user/pass for it.

add1sun’s picture

I was going back to reroll the schema docs patch (http://drupal.org/node/164983) and I noticed that block.install, filter.install, node.install, taxonomy.install and user.install do not seem to have made it in to HEAD. These are all new files created in the patch and schemas that are installed by system.install.

JirkaRybka’s picture

Missing files now fixed: http://drupal.org/cvs?commit=83359, so "needs work" stay only for documentation, I believe.

bjaspan’s picture

Excellent! Thanks for the commit; I will get to the doc changes shortly.

JirkaRybka’s picture

Thanks go to Dries ;) I just pointed that out.

bjaspan’s picture

Status: Needs work » Reviewed & tested by the community

I updated the Schema API handbook and posted an announcement on development@drupal.org. Unless someone can think of a location I'm forgetting, this should now be marked fixed.

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Fixed

Mark fixed, then.

Anonymous’s picture

Status: Fixed » Closed (fixed)