Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#30 | mysqli_errors.txt | 37.57 KB | dvessel |
#23 | drupal-schema-to-install-150245-23.patch | 125.96 KB | cburschka |
#15 | drupal-schema-to-install.txt | 124.59 KB | rstamm |
#14 | drupal-schema-to-install-150245-14.patch | 125 KB | cburschka |
#12 | drupal_schema_to_install-150245-12.patch | 105.63 KB | cburschka |
Comments
Comment #1
webchickAh, shoot. Didn't see this issue, so marked http://drupal.org/node/155220 as a duplicate.
Here's a patch.
Comment #2
bjaspan CreditAttribution: bjaspan commentedTest 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. :-)
Comment #3
webchickI 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.
Comment #4
webchickCrap. That didn't work for some reason.
Comment #5
webchickOk, 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.
Comment #6
webchickI 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.
Comment #7
bjaspan CreditAttribution: bjaspan commentedI 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.
Comment #8
RobRoy CreditAttribution: RobRoy commentedFWIW, I agree with bjaspan. hook_schema belongs in .install and the clarity far outweights the small overhead gain.
Comment #9
Dries CreditAttribution: Dries commentedI'd still like to commit this but it needs a re-roll for locale.schema and node.schema.
Comment #10
webchickHere we go; this cures the failed hunks. Tested clean install + enable all modules and it seems to work.
Comment #11
rstamm CreditAttribution: rstamm commented.schema files are protected from prying eyes now, http://drupal.org/cvs?commit=72331
Patch should remove it from .htaccess.
Comment #12
cburschkaSounds 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:
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.
Comment #13
cburschkaOops. 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.
Comment #14
cburschkaFiguring 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
Which together with the .htaccess change can roughly account for the difference, so I think this one's okay.
Comment #15
rstamm CreditAttribution: rstamm commentedPatch didn't apply anymore, I've rerolled it.
Comment #16
ChrisKennedy CreditAttribution: ChrisKennedy commentedNeeds to be re-rolled.
Comment #17
moshe weitzman CreditAttribution: moshe weitzman commentedWe are working down the Criticals queue in Barcelona. Since Drupal works fine without this change, it isn't Critical.
Comment #18
webchickThis is actually probably a won't fix issue at this point... I'll leave it up to bjaspan.
Comment #19
bjaspan CreditAttribution: bjaspan commentedI 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?
Comment #20
cburschkaWhat 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.
Comment #21
webchickThe 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.
Comment #22
cburschkaAnd 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...
Comment #23
cburschkaArgh. 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.
Comment #24
bjaspan CreditAttribution: bjaspan commentedA 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.
Comment #25
bjaspan CreditAttribution: bjaspan commentedIf 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.
Comment #26
Dries CreditAttribution: Dries commentedOK, I committed this patch. Thanks for all the work.
We'll want to update the documentation though. Therefore marking as 'code needs work'.
Comment #27
eaton CreditAttribution: eaton commentedI 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.
Comment #28
killes@www.drop.org CreditAttribution: killes@www.drop.org commentedIs it possible that this patch broke new installs? Only a part of the tables gets installed. Missign are for example users, blocks, and access.
Comment #29
killes@www.drop.org CreditAttribution: killes@www.drop.org commentedI think the patch is alright, but Dries forgot to "cvs add" the new install files...
Comment #30
dvessel CreditAttribution: dvessel commentedI have the same problem as killes.
Attached the errors spewing out. This happened after submitting the db name and user/pass for it.
Comment #31
add1sun CreditAttribution: add1sun commentedI 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.
Comment #32
JirkaRybka CreditAttribution: JirkaRybka commentedMissing files now fixed: http://drupal.org/cvs?commit=83359, so "needs work" stay only for documentation, I believe.
Comment #33
bjaspan CreditAttribution: bjaspan commentedExcellent! Thanks for the commit; I will get to the doc changes shortly.
Comment #34
JirkaRybka CreditAttribution: JirkaRybka commentedThanks go to Dries ;) I just pointed that out.
Comment #35
bjaspan CreditAttribution: bjaspan commentedI 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.
Comment #36
Gábor HojtsyMark fixed, then.
Comment #37
(not verified) CreditAttribution: commented