Use hooks instead of function calls

[leewillis77]

Opening this up for discussion - not sure if it's the "right" thing to do - but worth looking at.

If you look at how WooCommerce does their theming they lean heavily on hooks rather than direct function calls, e.g.

https://github.com/woothemes/woocommerce/blob/master/templates/content-single-product.php

So, in the template code instead of:

wpsc_breadcrumb();

We'd have:

do_action('wpsc_theme_breadcrumb'); and elsewhere: add_action('wpsc_theme_breadcrumb', 'wpsc_breadcrumb');

I'd suggest every theme related hook had a consistent prefix (wpsc_theme in the example above).

Pros:

• You get hooks-by-default everywhere you have any kind of generated output, without having to worry about adding ones you need - makes it more likely there will be hooks where people want to extend stuff
• People who don't want specific functionality can remove it with remove_action() rather than having to edit the theme files (Will this give us fewer upgrade hassles?)

Cons:

• Performance?

[benhuson]

I quite like this approach.

It's possible to pass arguments too eg:

do_action( 'wpsc_theme_breadcrumb', array(
   'separator' => '>'
) );

[garyc40]

I originally follow the same approach, using hooks in place of template tags (@JustinSainton might remember this because I had a long discussion with him and back then I decided to go with a bunch of hooks in the new templates).

But then after some further consideration I decided that using hooks is redundant, and switched back to just using direct calls to template tags instead. The reasons for this are as follows:

• It's a roundabout way of doing things. You create a hook, and you hook the function in. 99% of the time, the user doesn't need to touch the hook.
• If the user wants to disable breadcrumbs, then removing it via remove_action() is not an intuitive way to do that. I'd rather have a bunch of internal options (or UI in our settings page) that lets users toggle it on / off than calling remove_action(). A call to update_option( 'wpsc_enable_breadcrumb', false ) would be more intuitive than remove_action( 'wpsc_theme_breadcrumb', 'wpsc_breadcrumb' ).
• Extensibility, if there should be any, should be done from within the function itself, rather than queuing function before and after the default function in the same hook, or removing the default function completely and replace it with a new one.
• Huge code readability issue. You cannot see at a glance which function is being used behind that action hook. If you're debugging a site, and the breadcrumb is acting up, you would need to debug from within the do_action() function in order to determine which function is being hooked to that, whether it is using the default function, or someone has customized it with his own function. With a direct function call, although that could mean the user has to duplicate the template file and edit it, you can see right away if there's anything funny going on.
• IDE integration, documentation, and autocomplete. I haven't figured out how to provide inline documentation for the arguments that a hook accepts, how to go directly to the function definition that is hooked into that action etc. This point is related to code readability point above.
• More hooks actually means more compatibility issues down the road. There's no graceful way to deprecate hooks, but we can deprecate functions.
• I'm not saying bbpress and buddypress are the models that every single plugin has to follow, but they also use direct function calls rather than hooks.

That being said, the theme engine is still at a stage where we can conveniently switch to using hooks, as long as we have good reasons to do so. There might be pros / cons to using hooks vs using functions that I might not be aware of, so I'm interested in hearing more from the community.

[benhuson]

I've rewritten this comment 4 times as I can't make my mind up..... I probably contradict myself a lot in the below :)

I can see that debugging problems would certainly be more problematic with the actions approach, and deprecation is a valid point too - I was thinking that actions degrade gracefully by just not firing, but proper deprecation might be more difficult.

I think that having actions for things like 'before_product_summary' etc are handy for hook in to add extra content etc, but maybe function calls are better for the actual core output - the functions can include filters where appropriate.

I like actions as an idea when used simply like this:

<?php

do_action( 'woocommerce_archive_description' );

if ( apply_filters( 'woocommerce_show_page_title', true ) ) :
   // Something here
endif;

?>

As a theme developer this gives me an instance overview of how I can override the content of those functions without having to delve in the code. This works fine when returning fairly standard bits of visual content; text, images, galleries, returning true/false etc.

I'm not as keen when they start to look like this:

echo apply_filters( 'woocommerce_subcategory_count_html', ' <mark class="count">(' . $category->count . ')</mark>', $category );

I can see the benefit of something like echo apply_filters( 'wpsc_variation_groups' ); Someone who knows what they are doing could create a plugin that replaced the variation menus without having to make theme changes.

That said, I don't mind template tags as long as they have places to hook in or filter should I need to.

I don't know... both ways seem valid to me

[JustinSainton]

Yeah, I do recall our initial discussion on the topic. It's a tricky thing, because it really comes down to preference, not perfection.

Some of my general thoughts:

• Personally, I lean towards template tags in theme files. It seems more idiomatic to me as I read through a theme file. In those functions, I lean towards heavy extensibility. Essentially wrapper functions for actions and filters. If extensibility should be further exposed to the user via options, those options shouldn't show/hide functionality like we do now, they should hook/unhook from actions.
• I took a quick look at some bbPress template files. They seem to do both, actually. In the main file I looked at, there's both, but actions seemed to be more prevalent.
• In the example you gave, Ben, it would seem fine to me (as a compromise, to be clear :), it wouldn't be my preference) to have the do_action( 'wpsc_archive_description' ) call in the theme file - but in my mind, that if ( apply_filters[...] ) call would be better inside a wpsc_show_page_title() template tag.

[garyc40]

@benhuson: I agree that having a "guarding" filter at the beginning of the template tag could be a compromise worth considering:

function wpsc_get_some_template_tag( $args ) {
    // allow overriding the output of this template tag
    $output = apply_filters( 'wpsc_override_some_template_tag', false, $args );
    if ( $output !== false )
        return $output;

    // if the output is not overridden, proceed as usual
    $output = '...';
    return apply_filters( 'wpsc_get_some_template_tag', $output, $args );
}

function wpsc_some_template_tag( $args ) {
    echo wpsc_get_some_template_tag( $args );
}

But I don't think having an if ( apply_filters() )... else ... block in the template is a good thing.

@JustinSainton:

• Could you provide some example for hooking / unhooking from actions using options? If it is what I'm imagining it is, I still maintain that it's unintuitive and redundant:

function _wpsc_action_some_template_tag_default() {
    $output = '...';
    echo $output;
}

function wpsc_some_template_tag() {
    // this is basically the same thing as using hooks in template and still has the same problems
    if ( ! get_option( 'wpsc_enable_some_template_tag' ) )
        remove_action( 'wpsc_some_template_tag', '_wpsc_action_some_template_tag_default' );

    do_action( 'wpsc_some_template_tag' );
}

• Yes, bbPress has actions in their templates, but they are not actions that are used to call template tags. I have no problem at all with _before and _after hooks (although I'd prefer to tuck them inside template tag functions rather than leaving them scattered around in templates). What I was talking about was hooks that are used as placeholders for template tags like what Woo is doing. bbPress and buddyPress templates have _before and _after hooks and that's fine. What they don't have is this type of hook: do_action( 'bbp_breadcrumb' );, but instead they call bbp_breadcrumb() directly.

Extensibility is definitely important but adding hooks just for the sake of having them could create some problems with code maintenance in the long run.

It all comes down to this: What problem does using hooks in templates solve, that using direct template tags does not?

[benhuson]

The only plus side to hooks is that if you needed to use them in non-wpec generated pages, then later deactivated wpec, they would fail gracefully by default.

With templates tags you have to remember to check if function_exists() if you're using anywhere that might continue to show if the plugin was deactivated.

This is best practice anyway and I think I would prefer to stick to template tags and just use filters for placeholders such as before and after and within the template tag functions.

Having a "guarding" filter at the beginning of the template tag so that you can completely override it is good for some things such as product galleries so plugins can easily customise.

[agusmu]

+1 for @leewillis77 and @benhuson idea for hooks, both actions and filters... It is not only for end user, but also for theme&plugin developer.

[garyc40]

@agusmu : Could you please elaborate more on how using hooks instead of template tags is easier for theme & developers? My position is that it makes debugging a nightmare for developers (as explained above).

Note that using template tags inside templates doesn't mean you can't customize or filter the output of the template tags. The hooks are moved inside the template tags.

@benhuson : You made a valid point about fatal errors when WPEC is disabled, and I agree with you that there's a best practice to handle that situation anyways without having to use hooks, for the following reasons:

• Most wpec related template tags are supposed to be contained within WPEC templates. If it bleeds into other templates, then it probably indicates some deeper maintenance issues.
• You don't need to use function_exists() everywhere in the templates. You need to write a wrapper function for the template tags you want to use, and inside those functions, use function_exists(). That's the proper way of handling dependencies between theme and plugins.
• do_action() does not indicate dependency issues. It might appears "graceful" when a required dependency (in this case WPEC) is disabled. However it masks a deeper issue: the theme is supposed to require WPEC to be enabled. If it is disabled, and there's no indication of that requirement being broken, it's not a good approach.
• When we need to deprecate template_tags(), we can simply use wpsc_deprecated_function(). When we need to deprecate hooks, there's no way to indicate that.
• We might need to get @johnjamesjacoby involved with this discussion and ask him whether he made any conscious decision regarding template tags vs. hooks for bbpress and buddypress.

This is quite an essential issue that we need to have a consensus on so let's try to write down all the pros and cons then it's much easier to make informed decision based on that :)

Thanks for all the feedback so far.

[benhuson]

@garyc40 Agree with all your points:

• I was just thinking about plugins that may use template tags and specific theme customisations and trying to ensure they fail gracefully, but yes, it's up to whoever codes that to do function checks etc.
• Completely agree that functions are easier to deprecate

My preference is to use template tag functions for the most-part, using actions/filters within functions where needed, and only use action hooks in templates to allow plugins or extra functionality to hook in in certain places to add extra content. I'm thinking things like ratings, social icons etc, things that are not part of core ecommerce functionality.

[ryanhellyer]

My 20c worth ...

Many programmers will prefer the use of hooks to direct function calls as it makes a lot sense from a programming perspective. It allows you hook and unhook things willy nilly and gives you are lot more control and often cleaner code.

However ... n00bs aren't going to see any of those advantages. And in fact they'll probably find a bunch of do_action() calls kinda confusing as they won't understand what it's doing and will be simply copy/paste and modifying. Template tags are the simplest chunk of PHP you can use. They're usually fairly obvious how they work even to n00by developers. I believe this is why WordPress uses them so heavily within it's default theming system.

I see two main advantages of using template tags in WP e-commerce:

(1) They match how regular WordPress themes do things. This means that users will be more used to how it works and hopefully just figure it out immmediately without needing to do any learning. (2) How they work is more obvious for n00bs.

I see two main advantages of using action hooks in place of template tags in WP e-commerce

(1) Simpler to unhook stuff and save users editing their theme files (2) Slightly more useful for developers

To my mind, the advantages of template tags outweigh the benefits of using action hooks. I think it is particularly critical to remember how much difficulty users of this sort of software have with even extremely basic tasks, and adding in the complexity of understanding what an action hook is, may be a step further than is a good idea.

Any time you step outside of the "normal" WordPress conventions I think you should be very careful. Template tags are a WordPress convention.

[ryanhellyer]

Another 10c worth ....

There are of course times when action hooks make far more sense. For example if you need someone to add code to their theme, but that theme may need to work without your plugin loaded at some point. Then when they disable your plugin, their theme will fail. So in that situation the action hook makes far more sense as it won't serve a fatal error when the plugin is turned off. You can of course use if function_exists .. but that's a bit ugly. Action hooks make more sense in that situation IMHO.

[instinct]

1. Template tags are okay for hard coding when you know that the function will always be available, for example, core wordpress functions, but themes cannot hard-code a function call to a plugins function because if the plugin is deactivated or similar every things breaks
2. Hooks and filters are good for inserting into 3rd party code, for example, a users theme adding specific hooks to cause a plugin to output something at that point, it's almost the same as calling a plugin function, but, with the added benefit that if the plugin is deactivated, or something else happens, the hooks still run as normal, just nothing is output.
3. remove_action() should never be needed in that case, if a user doesn't want something output, then they remove the action from the code. but, if it's a 3rd party theme they're using thats being continuously updated, they've the flexibility that they can child-theme it, and add remove_action() calls into their child theme to override the parent, etc.
   • anonymous WP mate

[instinct]

WordPress themers will be more comfortable with direct function calls. Developers who are comfortable with filters, and have seen the destruction plugins becoming deactivated will love actions. If using actions, you can still call the function directly though, so really, it's win-win..

• same anonymous WP mate

[mfields]

There's a third solution here too: create custom template parts. You can create a template part for each module (for lack of a better word) that the plugin needs to introduce into the theme. These template parts would exist inside the plugin, but would be included using locate_template() which would provide both parent and child themes the ability to override them. simple example would look like this:

$template_name = 'my-custom-template.php'
$path = locate_template( $template_name );
if ( empty( $path ) ) {
    $path = dirname( __FILE__ ) . 'inc/' . $template_name;
}
include( $path );

This code could be wrapped in a function by the plugin and then called in the theme either directly or with an action. It doesn't really matter in this case imho because the function/action is not the "interface" and would not need to be modified by the themer. The themer would copy the template code from the plugin into the theme and then tweak it from there.

[benhuson]

@mfields I guess that's sort of how the new template engine works (see the template-parts https://github.com/wp-e-commerce/wpec-theme-engine-v2/tree/master/theming/template-parts) but at a more granular level?

[devinsays]

I agree with @ryanhellyer. If the theme is aimed at a developer crowd, hooks are great. If it's aimed at a broader audience, a straight function or get_template call easier to follow.

For what it's worth, this is how @justintadlock does it in Hybrid:

<?php if ( current_theme_supports( 'breadcrumb-trail' ) ) breadcrumb_trail( array( 'container' => 'nav', 'separator' => '>', 'before' => __( 'You are here:', 'hybrid-base' ) ) ); ?>

[JustinSainton]

I would personally be in favor of a hybrid approach - I think there's quite a bit to be gained from looking at the potential solutions as both/and, not either/or. I don't particularly care for templates that are essentially just action hooks (Which is what I think you originally referenced, @benhuson, with WooCommerce). It's simply not discernable or idiomatic for developers or non-developers.

I also don't like template tags that have no actions/filters. I think that's a waste and makes it impossible to have extensible themes outside of child theming. I'm not certain that anyone is actually proposing that, though. Based on my understanding (which, admittedly, is comparatively limited when compared to people like @mfields or @ianstewart or @johnjamesjacoby) of the way WP core works is that it uses all of these to achieve the most proper, idiomatic and extensible result. We should likely be doing the same.

Like @benhuson suggested, it does appear that we are currently using locate_template(). I think, my personal preference (which is only that, preference) at this point is to figure out the logic behind when to include a template tag within a template part, and when to simply include an action.

If we can infer that line of logic and reasoning from bbPress and BuddyPress (and hear from some of the folks on the theme team and core devs from those plugins - it would be great to know what about that approach works great and what could be improved upon) - I think that helps. But I definitely don't think it's a matter of either/or.

[benhuson]

I think it's a combination of template tags, hooks, and templates as @JustinSainton says depending on the output. Thing is figuring our what to use where. My quick thoughts are:

• Template tags feel like they should be used for any essential functionality; outputting prices, variations etc. These can be filterable within the function so you could output variation menu different for example. Template tags make sense where a simple output is expected (price, title, text) but the behind-the-scenes functionality could be quite complex like variations menus.
• Templates could be used for output mainly layout-based output that a user may want to change; galleries, cart, widgets etc
• Hooks could be used as placeholders for optional output that may be added/removed by plugins; ratings, social icons, extra content etc.

[agusmu]

I don't know if this discussion is the most important issue on this new theme engine... It is clear that we will not use fully hook system like what we see on WooCommerce OR fully function call system like what we see on current WPEC template system. We simply use combination of these like all of your suggestions above. winwin

I just tested the theme engine v2 and I can say that it really really big changes.

(1) What about backward compatibility?

(2) Now all Store pages are gone, the link is created automatically, instead of using shortcodes, (WooCommerce and others still use shortcode). On my several test, it is not easy for me to get Cart page running, okay I am newbie after all. Do really we need this?

(3) What about default view, list view, and grid view that WPEC offer on current release. Do you remove it at all?

(4) etc...

I know I should posted this on new issue, but I just want to say that my first experience for testing theme engine v2 is not great at all... There are many homework that need to do to answer before we continue the discussion about hooks and function calls...

I only want to see smooth transition from user experience view to move from current template system to new theme engine, then we can improve this new theme engine and continue this discussion...

[garyc40]

Yes you should post these issues in separate issues so that we can discuss further.

This hook discussion might not be as important as other pressing issues, but as you can see it's a dividing issue that we should just reach a solution and get it out of the way as soon as possible. This particular hook discussion actually makes me very happy as this shows people are passionate enough about WPEC's future to express their opinions, and for that I'm very grateful.

As for other issues you have posted, I'd like to just post a very short response here, but please do go ahead and create new discussions so that we can discuss more in depth there.

1. Backward compatibility: If you install this plugin, the old theme engine currently in WPEC 3.8.x will be deactivated automatically, and this one kicks in. Old templates won't work. In the future when we merge this plugin into core, the old theme engine will be spun out as a separate plugin, so that users who want to continue to use it can. We make this a clear break from the previous theme engine precisely because we want to start from scratch, with the community's feedback every step of the way. This is so that the community can mold it the way they prefer, rather than continuing to rely on a legacy template engine that has gone out of date and out of favor.
2. The cart page not running is potentially a bug. I might need more information on your setup (particularly your permalink scheme) in order to solve that (on a separate issue ticket). It's not that we're not going to support shortcode placeholders inside page any more. The old theme engine was designed so that it works as shortcodes inside pages, and this has created all kinds of compatibility issues with themes and the main query. It's the wrong approach. This time we're designing the theme engine independent of the shortcode system as it is actually easier to ensure compatibility that way. If in the future, community feedback indicates that they prefer shortcodes, we can add shortcode support back.
3. I must emphasize again that this theme engine, as long as it exists as a separate plugin from WPEC, is for developers and early-adopters only. The main purpose of this is not for widespread use yet, but mainly for gathering feedback, and for the community to let us know what they're looking for in a template engine. We will eventually add full features for default view, list view or other views. But for now this template engine remains minimal. We'll build it up block by block again.

This is a work in progress. But we're making it public early so that the whole developer and user community can jump in and give us feedback early, which is precisely what you're doing, and for that we all appreciated.

Please keep the feedback and comments coming in (in separate tickets though!). I'll be happy to answer your questions in more details there.

[JustinSainton]

I've referenced this conversation in https://github.com/wp-e-commerce/WP-e-Commerce/issues/848 - fantastic conversation and it should continue, if necessary, on that issue.