search

roots / sage

WordPress starter theme with Laravel Blade components and templates, Tailwind CSS, and a modern development workflow
https://roots.io/sage/
MIT License
12.77k stars 3.06k forks source link

do_action('get_header') should occur before wp_head() #3171

Closed kevinlisota closed 10 months ago

kevinlisota commented 11 months ago

Version

10.7.0

What did you expect to happen?

The default WordPress theme behavior is to load header.php using a call to get_header(). Sage doesn't do this but does include do_action('get_header') so that any plugins relying on that action (maybe to enqueue styles, scripts, or meta tags) still fire.

I've embedded a Gravity Form in a theme file using their function call. For this to work, you need to manually enqueue the required styles and scripts for the page where the form is appearing.

Gravity Forms is expecting the typical WordPress ordering, where the get_header action runs before wp_head(). The result should be that their styles get enqueued in wp_head(), before the Sage app.css.

What actually happens?

Sage does fire the get_header action in index.php, but after wp_head() and after the body_open. The result for this Gravity Forms use case is that the styles don't get enqueued until the footer, which messes up the stylesheet ordering.

While not difficult to work around for my use case, I'm wondering if the ordering of that action should change in index.php. If any other plugins are relying on the get_header action to enqueue styles, scripts, or other things that belong in wp_head(), the current order doesn't work because it is too late. The get_footer action does have the proper ordering.

Steps to reproduce

Use the get_header action to enqueue a style that should appear in the <head>. The style will appear in wp_footer() instead.

System info

No response

Log output

No response

Please confirm this isn't a support request.

No

retlehs commented 11 months ago

Gravity Forms is expecting the typical WordPress ordering, where the get_header action runs before wp_head().

This doesn't seem correct?

Ref https://github.com/WordPress/WordPress/blob/master/wp-content/themes/twentytwentyone/header.php Ref https://github.com/WordPress/WordPress/blob/4a146890164f80e595e80477be9f10ab78ff77a4/wp-includes/theme-compat/header.php

so that any plugins relying on that action (maybe to enqueue styles, scripts, or meta tags)

Hooking into get_header to enqueue assets is not the correct way to add assets. The best practice is to use the wp_enqueue_scripts hook to ensure proper compatibility and loading order.

kevinlisota commented 11 months ago

Looking at your example from TwentyTwentyOne, header.php is loaded through this call in index.php.

The get_header function first fires the get_header action. Then it locates the header template. So wp_head() occurs after the get_header action.

Haven't investigated why Gravity Forms gives this particular guidance for using that action hook. But I glanced at some other plugins, and the get_header does occasionally get used for other purposes as well. I see The Events Calender uses it for some json_ld items, for example.

I get that wp_enqueue_scripts is the correct place, but shouldn't the get_header action fire before wp_head() to address anyone using that action?

retlehs commented 11 months ago

It seems wrong that The Events Calendar would be hooking into get_header instead of wp_head to output that data

While Sage doesn't follow the standard WordPress theme behavior, these plugins aren't using the correct hooks for their functionality

kevinlisota commented 11 months ago

I don't disagree and haven't investigated exactly why they are using that hook. Some of my Woocommerce plugins use that hook too.

But I still think that the order in Sage's index.php template is wrong, and get_header action should occur before wp_head() in index.php.

I've modified our index.php template like this, moving that action out of the body:

    <?php do_action('get_header'); ?>
    <?php wp_head(); ?>
  </head>
Log1x commented 11 months ago

But I still think that the order in Sage's index.php template is wrong, and get_header action should occur before wp_head() in index.php.

I do not agree. get_header() (when not called in the context of the action) is simply a call to the header.php template.

What header.php consists of depends on how the theme is being developed and how get_header() gets called. You can see in the documentation's comments/replies the many use-cases people have had for this template.

This can vary from header.php consisting of your entire document start+head+beginning of the body tag to just your header/navigation.

Take this quick response from ChatGPT for example - it gives the example of putting wp_head() its self inside of the header.php template so it all gets called together to keep index.php DRY - which makes sense, and is fine. If a developer otherwise targeted get_header in this case, their code would get added after </header>.

In Sage's case, we do not need to do this as everything is otherwise pretty dry as-is thus we can omit having a header template in entirety. Since we do this, for obvious compatibility reasons, we have to do do_action('get_header') so any plugins (like these) still have their hooks properly ran. In this case, and your Events Calendar example, this stuff should definitely be hooked using wp_head.

I am very confident do_action('get_header') should be after wp_body_open() and if these plugins are enqueueing important assets/scripts through get_header and are not working in Sage's current configuration - they are doing it wrong.

kevinlisota commented 11 months ago

How the get_header action is being used by third parties is not my point. There may be valid cases and doing it wrong cases. Whether right or wrong, those plugin developers are expecting a certain ordering.

My point here is that the function get_header() is not simply a call to header.php. It calls the action hook get_header before loading the header template. And the definition of the get_header hook is "Fires before the header template file is loaded."

Sage doesn't need a header template but does include the get_header and get_footer actions for compatibility reasons. By putting it after wp_body_open() it occurs later than you'd see in a "typical" theme where get_header() gets called in index.php.

Even though Sage doesn't have a header template, my request is that Sage sticks to the hook definition of "Fires before the header template file is loaded" and execute it where other themes typically do, which is not after wp_body_open().

Log1x commented 11 months ago

Look at twentytwentyone (the last theme before they changed to block templates) as an example:

In what world would you want a plugin to run a hook that adds markup to your page before the opening of your document?

I get what you're saying, but this isn't getting changed in Sage. You will have to adjust this to your own needs or contact the plugin developers.

kevinlisota commented 11 months ago

I guess I should have been more precise in my analysis of what these plugins are doing to better explain the issue.

Looking at the four examples I have on hand, Gravity Forms, The Events Calendar, Query Monitor, and Woocommerce Subscriptions, and after a cursory examination of their code, here is what I found. None of them are emitting markup on that action hook, though at first glance a couple of function names were deceptive. As you mentioned and for obvious reasons, this is not a place to emit markup. They all are looking for a code execution point immediately before wp_head() to implement some logic that may or may not affect the markup that immediately follows. After further inspection, they may not be doing it wrong.

I did reference twentytwentone and the sequence it follows:

  1. index.php calls get_header()
  2. get_header() fires action get_header
  3. get_header() locates header template
  4. header template opens document and fires wp_head(), the opens body

In Sage, the get_header action in relation to wp_head() is out of sequence. For those who encounter this on plugins that use get_header, my solution was to move <?php do_action('get_header'); ?> before wp_head().

Log1x commented 11 months ago

That definitely makes more sense. I will look into this.

kevinlisota commented 11 months ago

To give a bit more context, the Gravity Forms example triggered this for me. I'm embedding a form in a template using their function call, not in a post. Because the form is not in a post, WordPress doesn't know about the form to enqueue their styles/scripts in the header because it hasn't gotten to that template code yet, so it needs to be done manually. (They only enqueue when a form is present on a post.) They instruct you to run their enqueuing function immediately before wp_head() so that the styles/scripts are enqueued when it comes time to emit. I suppose it could be done with priority numbers in wp_head, but the get_header hook seems more resilient which is probably why they chose it.

In the case of The Events Calendar, they use the get_header hook to check for the existence of wp_head hook immediately before it appears. (Because they need wp_head to output their json_ld. If wp_head hook doesn't exist in a theme, they add it.

Query Monitor seems to use get_header as a marker in their backtraces. I can't interpret what WooCommerce is doing on that hook. Has something to do with payment locks.

Log1x commented 10 months ago

Thanks a lot for providing more info on this. Sorry about the initial confusion.

Can you try #3167 and see if everything works as intended with your plugins? Given your plugins make extra-use of some of these hooks, you confirming that PR works will help a ton and I can go ahead and merge it in.

Log1x commented 10 months ago

3167 was merged but please let me know if you still experience any issues.

kevinlisota commented 10 months ago

Thanks. I'll be giving it a test later today and will let you know if any issues.