Here's some initial docs for the newly committed hook install.

CommentFileSizeAuthor
#7 install_php.patch3.01 KBhunmonk
#3 document_install_hook_0.patch1.99 KBdrewish
document_install_hook.patch1.88 KBdrewish

Comments

drumm’s picture

drumm
he/him
Location NY, US
commented

the mysql is missing the {} and I'm not so sure about the heredoc strings, we don't really use those anywhere now.

Jaza’s picture

Jaza commented

My only concern is site admins dealing with complex multisite setups, where multiple database prefixes are involved. This documentation needs to include a comment to such people, that if they want the tables created by hook_install() to use a prefix other than the default one, they must specify this in the relevant settings.php file before enabling the new module.

Perhaps we should also recommend that module developers list all tables in their module's installation instructions, along with instructions for custom prefixing.

drewish’s picture

drewish commented
StatusFileSize
new document_install_hook_0.patch1.99 KB

drumm, I added the {} to the MySQL case and a node that all tables need the curly braces so prefixing works correctly. You're right that we don't use the HEREDOC format much but it really makes it easier to paste in the SQL and to read it.

Jaza, you're right that this should be documented, but in the docs for end users, not developers. I think most people who'll be using the prefixes will set those up before downloading and enabling contrib modules. At that point all the tables created will have the proper prefixes. If a user need to change them later, they can open up the .install file, look at the module_install() function and see a list of the tables created.

drumm’s picture

drumm
he/him
Location NY, US
commented

Looks good.

Whenever this happens go ahead and cvs rm update.php, mv update.php install.php, cvs add install.php.

drewish’s picture

drewish commented
Status: Needs review » Fixed

This has been committed. The file was renamed as drumm suggested.

yktdan’s picture

yktdan commented

There needs to be a reminder somewhere to fix README.txt and/or INSTALL.txt in modules so that don't tell you to run the .mysql file which isn't there any more, e.g. event.

hunmonk’s picture

hunmonk commented
Status: Fixed » Needs review
StatusFileSize
new install_php.patch3.01 KB

@yktdan: the reminder shouldn't go in the hook doc, but on in the 'upgrading to drupal 4.7' section of the drupal handbook.

i found this doc to be just a bit confusing, so i've attached a patch which i think makes things a bit more clear. i removed the PHP heredoc, which i've never seen used in any drupal table creation statements before--it definitely threw me off for a sec. :) also cleaned up the spacing.

while playing with the install hook last night to get it working for my modules, i discovered that you can do lots more w/ it than just install db tables. the function is called from a full bootstrap, and even the functions of the module that was just enabled are available! i figured this would be a nice thing to note in the doc, so it's added as well.

drewish’s picture

drewish commented

+1 the changes look good to me.

m3avrck’s picture

m3avrck commented

Shouldn't that be 'CREATE TABLE IF NOT EXISTS' for MySQL?

And for examples, after talking with hunmonk, we came up with some solid examples in contrib, for eventrepeat.module and tinymce.module

eventrepeat.install -
http://cvs.drupal.org/viewcvs/drupal/contributions/modules/eventrepeat/e...

tinymce.install - http://cvs.drupal.org/viewcvs/drupal/contributions/modules/tinymce/tinym...

drewish’s picture

drewish commented

m3avrck, I don't think we need the "CREATE TABLE IF NOT EXISTS". The only time this is called is the first time the module is enabled. Even if by some chance you added the table and never enabled the module, the statement just fails, with no harm done. Besides, we don't do that in the current .mysql files.

m3avrck’s picture

m3avrck commented

drewish, yes this is true, this only ones once on install. However, it seems to me better practice to put in the "IF NOT EXISTS" instead of having the query explicity fail with warnings/errors everywhere.

However, as both hunmonk and I put in the eventrepeat and tinymce modules, we check for such errors and output a more friendly message. This should be done at a minimum and would suffice if the "IF NOT EXISTS" was not there...

drewish’s picture

drewish commented
Status: Needs review » Active

i committed hunmonk's patch (with some changes to the line wrapping) as several people didn't like the HEREDOC.

i'm going to leave the issue open though because i think m3avrck's point about the error checking/handling is a good one.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented

I'm going to actually disagree with m3avrck on the 'if not exists' addition. I agree it's nice that you don't get horrible errors everywhere, but the thing is, you probably WANT to get those horrible errors everywhere if the table you're trying to create already exists. This means either there's a naming conflict with another module, or somehow table prefixes are not being applied, etc. It seems like it would be difficult to troubleshoot if the installer went by with no errors but you were still getting 'field foo not found in query blah' when trying to perform certain actions, which is more difficult to track down.

drewish’s picture

drewish commented
Status: Active » Fixed

seems pretty settled.

Anonymous’s picture

(not verified) commented
Status: Fixed » Closed (fixed)