Hook into wp_head() and wp_footer() #4

Open
wants to merge 2 commits into
from

Conversation

Projects
None yet
7 participants
Contributor

obenland commented Jul 6, 2012

Guidelines:

The following template tags and hooks are required to be included where appropriate:

  • wp_head() – (immediately before </head>)
  • wp_footer() – (immediately before </body>)

Hooking 'tha_head_bottom' to 'wp_head' and 'tha_footer_after' to 'wp_footer' will respect that.

zamoose was assigned Jul 6, 2012

Owner

zamoose commented Jul 6, 2012

Hmmm. Convince me here. I was thinking of those two hooks as being distinct from wp_head and wp_footer to offer other options. However, if we're going to hook into the Core hooks, why not simply suggest that folks hook into the Core hooks with a high priority, as your patch does?

Contributor

obenland commented Jul 7, 2012

Having both seems redundant to me, while not having 'tha_head_bottom' and 'tha_footer_after' would be inconsistent with other hook types.

This would act like 'widgets_init' on 'init' or 'wp_enqueue_scripts' on 'wp_head'.

mrwweb commented Jul 10, 2012

I came here to write about this issue and found you guys talking about it! It seems redundant to me too (I would personally just drop them). Admittedly WordPress does do similar things, both with hooks like @obenland says, but also with tons of wrapper functions (like "add_theme_page", etc., so I don't think it's a huge deal.

But, since this is [re]starting now and everyone will have to learn it from scratch, maybe it's worth making it in the "ideal" way. (Or, to put it another way, maybe you give precedence to "don't duplicate core" over "be consistent with before/after." That's a type of consistency itself, though not as intuitive.) How many people using that_after_head would be able to handle using that hook but wouldn't know wp_head()?

Owner

zamoose commented Jul 10, 2012

@mrwweb I take your meaning. I guess I've been thinking of this as almost an HTML-like approach, where the hooks offer pseudo-wrappers, giving authors consistent behavior across all hooks and semantic areas

Now, obviously, wp_head and wp_footer are very special one-off edge cases. They have a very defined place in the theme hierarchy and themes that don't implement them are decidedly doing_it_wrong(). So, let's try for a use case model instead of just talking in hypotheticals:

Is there a use case where having distinct tha_head_bottom and tha_footer_after would present us with an advantage over simply directing devs to target wp_head and wp_footer with a high priority?

My thinking is: there's no practical use case, but rather a semantic one.

Contributor

obenland commented Jul 10, 2012

I don't know about tha_head_bottom, but tha_footer_after might make sense, when the footer is further wrapped in other tags.
Example? Sure: https://github.com/obenland/the-bootstrap/blob/master/footer.php

Contributor

chipbennett commented Aug 29, 2012

I think wp_head() should supersede both tha_head_top and tha_head_bottom. If anyone needs to hook into the document head, they can (and should) do so via wp_head (or any of the other hooks that fire within wp_head, as appropriate).

I think that the site footer and the document footer are two semantically different things. The core wp_footer() hook is intended to fire code in the document footer, while the tha_footer_after hook is intended to fire code after the site footer. Thus, I think both should be retained.

Owner

zamoose commented Aug 31, 2012

Chip:
What about if you need to inject something directly after the <html>? wp_head wouldn't allow for that, methinks.

Contributor

chipbennett commented Aug 31, 2012

What about if you need to inject something directly after the <html>?

What would the use cases be? When does something absolutely have to be injected immediately after the <html> tag? i.e. not inside either the <head> tag or the <body> tag?

And, in such a use-case, shouldn't the semantic hook name be something like tha_html_top or tha_head_before? Both of the current hooks, tha_head_top and tha_head_bottom fire inside of the HTML <head> tag. And I'm struggling to come up with an instance that anything fired via either of these two hooks couldn't equally well be fired via wp_head.

I think Obenland's plan of hooking the tha_head_bottom into the WP hooks is good for consistency, and is similar to the way WP handles things. The tha_footer_after isn't correct tho.

wp_head is effectively the same (location-wise) as tha_head_bottom (and should be hooked to wp_head), but not necessarily tha_head_top as themers tend to hard code stuff prior to wp_head.

wp_footer is effectively tha_body_bottom (not implemented, but should be hooked to wp_footer) not tha_footer_after. ( header and footer are html elements that can be located anywhere in the body's DOM structure, and aren't necessarily near the top or bottom of the body element.)

I don't think tha_thml_top is valid. Elements should be in head or body.

A tha_html_before (for doctype) would be nice, and a tha_body_top could be useful for some google recommendations.

Owner

zamoose commented Oct 30, 2012

Hmmm. tha_html_before() could be interesting, but it would have to be implemented carefully -- if not placed correctly by theme devs, you could end up with spurious whitespace before the element, no?

I don't think white space before the HTML would hurt anything. If you mean php headers already sent issues, that wouldn't be a problem either.

Just occurred to me that you could alternatively hook wp_head into tha_head_bottom instead of the other way around.

@zamoose Turns out there is an IE bug where if whitespace precedes the Doctype, then it enters quirksmode. But themer's would only have to be as careful with this as they've always been, no? Does remind me to bring up that it should probably a filter, not an action, as printing multiple doctypes isn't desirable.

Anyway, perhaps tha_html_before should not be standardized (leave it to the themer, not plugins). Just being thorough.

Just to be clear on this, it's okay to have tha_body_bottom inserted between wp_footer and the closing </body> tag? My understanding was that wp_footer had to be immediately before </body>. Will I have issues getting my theme approved for the repository with this code inserted?

Cais commented Nov 2, 2012

Although the guidelines state wp_footer should be directly before the
closing body tag, there are reasonable use cases where other "code" could
be placed there. One of the more common items that comes to mind is using a
CSS container to encapsulate the entire theme, thus if tha_body_bottom is
being used in that manner it would be no different than directly writing
CSS containers instead. I would not resolve the theme as "not-approved"
based on that alone.

Although you might keep in mind to add a comment to the submission ticket
regarding the implementation of "THA" hooks in the theme and note where
there could be concerns such as this one.

Cais.

On Thu, Nov 1, 2012 at 9:24 PM, Philip Newcomer notifications@github.comwrote:

Just to be clear on this, it's okay to have tha_body_bottom inserted
between wp_footer and the closing tag? My understanding was that
wp_footer had to be immediately before . Will I have issues
getting my theme approved for the repository with this code inserted?


Reply to this email directly or view it on GitHubhttps://github.com/zamoose/themehookalliance/pull/4#issuecomment-10002216.

Building on Cais's use-case (wrapping the inner-body with a container for CSS targeting), we can also consider JS targeting the wrapping container.

The reason why we place script tags at the end of the body tag, is so that all DOM element that effect rendering (CSS) and elements that will be manipulated with JS are loaded and constructed, prior to that script downloading and executing, i.e. prevent blocking.

The WordPress convention for this is for wp_footer to be outside of and below all other (renderable/targetable) html tags and for wp_footer to be the hook that script echoing is attached to. For this reason, I suggest moving tha_body_bottom above it and treat that hook as the one appropriate for echoing wrapper divs, lightbox markup, or what-have-you.

Obenland's patch would need a refresh to use the new hook

@WraithKenny WraithKenny pushed a commit to WraithKenny/themehookalliance that referenced this pull request Nov 2, 2012

Ken Newman fixes #4
Makes placement of `tha_footer_bottom` and `tha_head_bottom` optional,
falling back to @obenland idea of hooking into the corresponding wp
standard hooks. Priority of `tha_body_bottom` moved higher then default
`wp_footer`. Also, a few whitespace cleanups.
662b2d1

@WraithKenny WraithKenny added a commit to WraithKenny/themehookalliance that referenced this pull request Nov 2, 2012

@WraithKenny WraithKenny `wp_footer` below `tha_body_bottom`
See comments on #4
b04969e
function tha_body_bottom() {
    if ( current_theme_supports( 'tha_hooks', 'body' ) && ! did_action( 'tha_body_bottom' ) )
        do_action( 'tha_body_bottom' );
}
add_action( 'wp_footer', 'tha_body_bottom', 1 );

and

function tha_head_bottom() {
    if ( current_theme_supports( 'tha_hooks', 'body' ) && ! did_action( 'tha_head_bottom' ) )
        do_action( 'tha_head_bottom' );
}
add_action( 'wp_head', 'tha_head_bottom', 1 );

is my proposed solution on this. I can't seem to write a clean patch, so I'll just leave it here. (rough day).

Explanation: Hooks to the appropriate wp hook. Won't run twice if it was manually add to the theme, or if the theme doesn't declare support. Priority 1 used to get it to run earlier then scripts at 10.

Contributor

chipbennett commented Nov 2, 2012

I think it is important to remember that wp_footer() is not intended to inject HTML content into the template, but rather is intended primarily for linking footer scripts. That is the primary reason that the Theme Review Guideline is worded the way that it is.

Thus, a template location hook, which is intended to inject HTML content, should fire before wp_footer().

Just because Themes and Plugins are _doing_it_wrong() by using wp_footer() to inject HTML content doesn't mean that we should facilitate or support it.

That's imposing a somewhat stricter interpretation then is warranted. Outputting HTML, such as the admin-bar, in the wp_footer is not _doing_it_wrong() per se.

(Aside: I would argue for admin-bar to be added via jQuery rather then printed to the source, but that's a different thing.)

As mentioned above, care should be taken so that scripts intended for inclusion directly before the closing body tag (or more specifically, after elements intended to be manipulated) are handled correctly. This standard should promote such best practices.

I've submitted a pull for moving wp_footer below the tha_body_bottom and the code I suggested here has a earlier priority then 'wp_print_footer_scripts' (20) which I would suggest is the actual hook not intended for markup injection.

It's also worth noting that Core prints admin bar HTML after the scripts (priority 1000). This isn't a violation as the consideration of blocking isn't impacted:

Scripts not having to be aware of the admin bar are printed first so the download of those links can begin that much sooner, and the relevant DOM is available. Scripts dealing with the admin-bar are jQuery ready event triggered, and so the DOM will be available on time.

As a side effect, writing a wrapper that's intended to also wrap the admin-bar, would require hooking wp_footer at higher then 1000 and echoing the closing tag.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment