Book module is shown in the example, but it no longer implements hook_init().

dblog.module does, to add CSS, but i suspect it's not following best practice.

Should dblog instead be using '#attached' to add CSS to its page's render array?

And if adding CSS and JS no longer a recommended use for hook_init(), is there another example out there? locale or overlay module perhaps?

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jhodgdon’s picture

Title: Documentation problem with hook_init: what's the recommendation for conditional CSS? » Clean up hook_init() and hook_boot() doc
Issue tags: -CSS, -stylesheets, -dgd7, -best practices

Let's keep this issue as related to documentation only. File a separate issue to ask about what dblog should be doing.

I agree, the example in hook_init() is bad.
http://api.drupal.org/api/drupal/modules--system--system.api.php/functio...
The CSS is now being added by the book.info file I think.

The doc for hook_init() has some other problems too, which we should fix, such as that the first line should be one sentence, and hook_boot() should have () after it to make it into a link. hook_boot() is also lame in the same way, and both have grammar issues.

It appears that there are several example implementations of hook_init() and hook_boot() on api.drupal.org, so we should be able to come up with better examples.

jhodgdon’s picture

Status: Active » Needs review
FileSize
2.67 KB

It looks like someone changed the example in hook_init() already. How are all of these documentation issues getting fixed without getting into the documentation queue??? I looked at 3 issues just now that had all been fixed. Mysterious.

Anyway, there were still some cleanup issues for these two hooks, so here is a patch.

jhodgdon’s picture

#2: 1008250.patch queued for re-testing.

mlncn’s picture

Status: Needs review » Reviewed & tested by the community

The cleanup is good and the patch applies fine!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 1008250.patch, failed testing.

jhodgdon’s picture

Status: Needs work » Needs review

That failure is bogus. Doc-only patch causing fatal error?

jhodgdon’s picture

#2: 1008250.patch queued for re-testing.

jhodgdon’s picture

Version: 7.x-dev » 8.x-dev

At this point, this is d8 material. Hopefully that will launch a retest.

jhodgdon’s picture

#2: 1008250.patch queued for re-testing.

xjm’s picture

Issue tags: +Novice

Went to open an issue for this, and hey, it exists. :)

+++ modules/system/system.api.php	12 Jan 2011 05:47:27 -0000
@@ -672,7 +672,7 @@
+ * hook_init() instead. Thats the usual case. If you implement this hook

This needs to be "That's" or (better) "That is".

Other than that, the patch will need to be rerolled in the new format. This looks like a reroll a novice could handle.

xjm’s picture

Status: Needs review » Needs work
bxtaylor’s picture

Status: Needs work » Needs review
FileSize
2.72 KB

Rerolled: changed Thats to That is and changed non cached to non-cached. Here's the patch.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs backport to D7

Looks good to me! Should be d7/8. Thanks!

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 7.x and 8.x. Thanks.

Status: Fixed » Closed (fixed)
Issue tags: -Novice, -Needs backport to D7

Automatically closed -- issue fixed for 2 weeks with no activity.