sidebar replacement fires on get_header action?

[ivanblagdan]

Im doing some ajax stuff with a theme in which im not processing certain parts of the template, which skips the get_header action. Any reason Woo_Sidebars::init_sidebar_replacement() and Woo_Conditions::get_conditions() are being fired on the get_header action? Having it fire on template_redirect seems just as good and it would be safe against some template insanity like the one im doing.

Heres a diff of what would work in this case:

diff --git a/content/plugins/woosidebars/classes/class-woo-conditions.php b/content/plugins/woosidebars/classes/class-woo-conditions.php
index c5ed402..6cea712 100644
--- a/content/plugins/woosidebars/classes/class-woo-conditions.php
+++ b/content/plugins/woosidebars/classes/class-woo-conditions.php
@@ -70,7 +70,7 @@ class Woo_Conditions {
            add_action( 'admin_print_scripts', array( $this, 'enqueue_scripts' ), 12 );
        }

-       add_action( 'get_header', array( $this, 'get_conditions' ) );
+       add_action( 'template_redirect', array( $this, 'get_conditions' ) );

        add_action( 'wp_ajax_woosidebars-toggle-advanced-items', array( $this, 'ajax_toggle_advanced_items' ) );
    } // End __construct()
diff --git a/content/plugins/woosidebars/classes/class-woo-sidebars.php b/content/plugins/woosidebars/classes/class-woo-sidebars.php
index c94b96e..0f6b866 100644
--- a/content/plugins/woosidebars/classes/class-woo-sidebars.php
+++ b/content/plugins/woosidebars/classes/class-woo-sidebars.php
@@ -101,7 +101,7 @@ class Woo_Sidebars {
        add_filter( 'enter_title_here', array( $this, 'enter_title_here' ) );
        add_filter( 'post_updated_messages', array( $this, 'update_messages' ) );
        add_action( 'widgets_init', array( $this, 'register_custom_sidebars' ) );
-       add_action( 'get_header', array( $this, 'init_sidebar_replacement' ) );
+       add_action( 'template_redirect', array( $this, 'init_sidebar_replacement' ) );

        if ( is_admin() ) {
            global $pagenow;

[mattyza]

Hi @ivanblagdan.

Pre-release, this was hooked on template_redirect. Sure, it does make sense to me as well. Some conditional tags weren't available on template_redirect, so we use get_header instead.

get_header is a WordPress coding standard and should be adhered to, in terms of the WordPress theming guidelines. Given that get_header() is a core template tag, I'd advise using this, or at least calling the get_header action appropriately in your theme.

Your template modifications, however you're making them, shouldn't impact WooSidebars at all, if done in accordance with the WordPress coding standards.

[ivanblagdan]

I understand that its part of a regular template output as is the case with the theme i'm working. I'm also aware of the coding standards and Im not sure that coding standards have anything to say about partial output like this in. I'd argue that fetching sidebar widgets via ajax is a perfectly acceptable practice, although something you might choose not to support.

I might have time for a patch working around any unavailable conditonals via the template_redirect hook, would you accept it? IMHO It really does make sense functionally.

On Oct 17, 2013, at 11:00 PM, Matty [email protected] wrote:

Hi @ivanblagdan.

Pre-release, this was hooked on template_redirect. Sure, it does make sense to me as well. Some conditional tags weren't available on template_redirect, so we use get_header instead.

get_header is a WordPress coding standard and should be adhered to, in terms of the WordPress theming guidelines. Given that get_header() is a core template tag, I'd advise using this, or at least calling the get_header action appropriately in your theme.

Your template modifications, however you're making them, shouldn't impact WooSidebars at all, if done in accordance with the WordPress coding standards.

— Reply to this email directly or view it on GitHub.

[mattyza]

If you've got time for a patch, I'll be happy to review it. :)

Matt Cohen Chief Product Officer at WooThemes

http://woothemes.com/ http://matty.co.za/

On Thursday 17 October 2013 at 2:16 PM, ivanblagdan wrote:

I understand that its part of a regular template output as is the case with the theme i'm working. I'm also aware of the coding standards and Im not sure that coding standards have anything to say about partial output like this in.
I'd argue that fetching sidebar widgets via ajax is a perfectly acceptable practice, although something you might choose not to support.

I might have time for a patch working around any unavailable conditonals via the template_redirect hook, would you accept it? IMHO It really does make sense functionally.

On Oct 17, 2013, at 11:00 PM, Matty <[email protected] (mailto:[email protected])> wrote:

Hi @ivanblagdan.

Pre-release, this was hooked on template_redirect. Sure, it does make sense to me as well. Some conditional tags weren't available on template_redirect, so we use get_header instead.

get_header is a WordPress coding standard and should be adhered to, in terms of the WordPress theming guidelines. Given that get_header() is a core template tag, I'd advise using this, or at least calling the get_header action appropriately in your theme.

Your template modifications, however you're making them, shouldn't impact WooSidebars at all, if done in accordance with the WordPress coding standards.

—
Reply to this email directly or view it on GitHub.

— Reply to this email directly or view it on GitHub (https://github.com/woothemes/woosidebars/issues/19#issuecomment-26553754).

[ivanblagdan]

Great :) If you have any info on what was broken back then it would be great, AFAIK template_redirect should be the safe action to start using conditionals.

Thanks.

On Oct 17, 2013, at 11:19 PM, Matty [email protected] wrote:

If you've got time for a patch, I'll be happy to review it. :)

Matt Cohen Chief Product Officer at WooThemes

http://woothemes.com/ http://matty.co.za/

On Thursday 17 October 2013 at 2:16 PM, ivanblagdan wrote:

I understand that its part of a regular template output as is the case with the theme i'm working. I'm also aware of the coding standards and Im not sure that coding standards have anything to say about partial output like this in. I'd argue that fetching sidebar widgets via ajax is a perfectly acceptable practice, although something you might choose not to support.

I might have time for a patch working around any unavailable conditonals via the template_redirect hook, would you accept it? IMHO It really does make sense functionally.

On Oct 17, 2013, at 11:00 PM, Matty <[email protected] (mailto:[email protected])> wrote:

Hi @ivanblagdan.

Pre-release, this was hooked on template_redirect. Sure, it does make sense to me as well. Some conditional tags weren't available on template_redirect, so we use get_header instead.

get_header is a WordPress coding standard and should be adhered to, in terms of the WordPress theming guidelines. Given that get_header() is a core template tag, I'd advise using this, or at least calling the get_header action appropriately in your theme.

Your template modifications, however you're making them, shouldn't impact WooSidebars at all, if done in accordance with the WordPress coding standards.

— Reply to this email directly or view it on GitHub.

— Reply to this email directly or view it on GitHub (https://github.com/woothemes/woosidebars/issues/19#issuecomment-26553754).

— Reply to this email directly or view it on GitHub.

[mattyza]

@ivanblagdan No immediate thoughts on that. Looking forward to testing your pull request. :)

[ivanblagdan]

@mattyza I'd gladly fix the problem, if I could find it.

I've compared the state of conditions obj at the replace_sidebards() function with/without that diff i sent. Made different quieries for your conditions obj to work on. I didn't find any differences there that indicate something you are using is different between these two hooks. This is the quickest test i could think of atm :)