cdils / utility-pro

Utility Pro Theme
GNU General Public License v2.0
14 stars 5 forks source link

Update theme version from style.css and use CONSTANTS for some functions. #100

Closed jdelia closed 7 years ago

jdelia commented 7 years ago

I've added some code to my Utility Pro functions.php that retrieves the version numberfrom the stylesheet vs a hard coded CONSTANT. I also created two more CONSTANTS for get_stylesheet_directory() and get_stylesheet_directory_uri() and use those CONSTANTS vs the functions throughout the theme. I was told that it is more efficient to use the CONSTANT vs calling the function over and over.

If you like I could submit the code as a pull request. If so, how would you like me to do that?

GaryJones commented 7 years ago

Genesis already defines CHILD_DIR and CHILD_URL. Relying on them can only be done once Genesis is setup, i.e. not in global scope.

Constants are more efficient than function calls, but pinning a value down can make testing more difficult. See https://core.trac.wordpress.org/ticket/18298 for a similar discussion. In this case though, Utility Pro defines a hard-coded version which means manually updating during the release procedure, but means it avoids a file open call every single time a page is loaded. It doesn't change often, so it doesn't need to be dynamically read from the style sheet.

With the fact that v2 doesn't have style.css at the start (because it's built via gulp), then code that tries to read the version from style.css would need to be defensive and have a fallback. Right now, the version value that eventually appears in the generated style.css is set in package.json and appears via the Gulpfile.js

jdelia commented 7 years ago

Thanks @GaryJones - This is what I am using for mine. The version that WordPress already has without a separate call. If DEBUG is on however, I generate a version based on the file time.

// If WP_DEBUG is on, this adds unique string to css file reduce stylesheet cached issues during development.
if ( WP_DEBUG ) {
    define( 'CHILD_THEME_VERSION', filemtime( CHILD_THEME_DIR . '/style.css' ) );
} else {
    define( 'CHILD_THEME_VERSION', $child_theme->get( 'Version' ) );
}
GaryJones commented 7 years ago

And how are you populating the $child_theme object?

jdelia commented 7 years ago

Sorry - that was something that Tonya demonstrated in KTC.

$child_theme = wp_get_theme();
GaryJones commented 7 years ago

wp_get_theme has two further function calls, and then instantiates a WP_Theme object with it's own non-trivial constructor.

The point of constants, is that that are ... constant. Something like SECONDS_PER_HOUR is not going to change, regardless of the context.

What are you defining a child theme version for?

If it's for built-in script (enqueueing) versioning, then how about conditionally setting the script version there instead? Or better yet, giving each built-in script its own version number, that need not change between UP releases AND optionally setting that to filemtime when WP_DEBUG is defined and truthy?

i.e.

    // Keyboard navigation (dropdown menus) script.
    \wp_enqueue_script(
        'utility-pro-keyboard-dropdown',
        \get_stylesheet_directory_uri() . '/js/keyboard-dropdown' . $suffix,
        [ 'jquery' ],
        \CHILD_THEME_VERSION,
        true
    );

becomes:

    $keyboard_dropdown_version = defined( 'WP_DEBUG' ) && WP_DEBUG ? filemtime( 'path/to/js/keyboard-dropdown' . $suffix ) : '1.2.3';
    // Keyboard navigation (dropdown menus) script.
    \wp_enqueue_script(
        'utility-pro-keyboard-dropdown',
        \get_stylesheet_directory_uri() . '/js/keyboard-dropdown' . $suffix,
        [ 'jquery' ],
        $keyboard_dropdown_version
        true
    );