Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Closed
kevinlisota opened this issue Jan 1, 2024 · 13 comments · Fixed by #3167
Closed

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

kevinlisota opened this issue Jan 1, 2024 · 13 comments · Fixed by #3167
Labels
bug

Comments

@kevinlisota
Copy link
Contributor

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
@kevinlisota kevinlisota added the bug label Jan 1, 2024
@retlehs
Copy link
Member

retlehs commented Jan 1, 2024 •
edited
Loading

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
Copy link
Contributor Author

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
Copy link
Member

retlehs commented Jan 1, 2024

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
Copy link
Contributor Author

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
Copy link
Member

Log1x commented Jan 1, 2024 •
edited
Loading

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
Copy link
Contributor Author

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
Copy link
Member

Log1x commented Jan 1, 2024 •
edited
Loading

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

  • header.php contains the start of the document, head, beginning of the body.
  • index.php calls get_header() similar to my ChatGPT 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.

@Log1x Log1x closed this as completed Jan 1, 2024
@kevinlisota
Copy link
Contributor Author

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 Log1x reopened this Jan 2, 2024
@Log1x
Copy link
Member

Log1x commented Jan 2, 2024

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

@kevinlisota
Copy link
Contributor Author

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 added a commit that referenced this issue Jan 4, 2024
@Log1x
🩹 Move get_header hook above wp_head() (Fixes #3171)
94a1787
@Log1x
Copy link
Member

Log1x commented Jan 4, 2024 •
edited
Loading

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 Log1x mentioned this issue Jan 4, 2024
Merged
@Log1x
Copy link
Member

Log1x commented Jan 5, 2024

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

@kevinlisota
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

🎨 Move default theme markup to the app layout roots/sage
3 participants
@retlehs @kevinlisota @Log1x