Skip to content

Add asset cache version helper function. #585

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

Closed
kopepasah wants to merge 4 commits into from
Closed

Add asset cache version helper function. #585

Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a7a540c
440 Add asset cache version helper function.
kopepasah May 1, 2017
8642ea1
Merge branch 'master' into add/440-script-versioning-cachebust
kopepasah May 1, 2017
4a67289
440 Simplify conditional checks and use empty check.
kopepasah May 1, 2017
4714c93
Merge branch 'master' into add/440-script-versioning-cachebust
kopepasah Sep 23, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 39 additions & 6 deletions index.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,12 +43,12 @@ function gutenberg_register_scripts() {

// Editor Scripts.
wp_register_script( 'tinymce-nightly', 'https://fiddle.azurewebsites.net/tinymce/nightly/tinymce.min.js' );
wp_register_script( 'wp-i18n', plugins_url( 'i18n/build/index.js', __FILE__ ), array(), filemtime( plugin_dir_path( __FILE__ ) . 'i18n/build/index.js' ) );
wp_register_script( 'wp-element', plugins_url( 'element/build/index.js', __FILE__ ), array( 'react', 'react-dom', 'react-dom-server' ), filemtime( plugin_dir_path( __FILE__ ) . 'element/build/index.js' ) );
wp_register_script( 'wp-blocks', plugins_url( 'blocks/build/index.js', __FILE__ ), array( 'wp-element', 'tinymce-nightly' ), filemtime( plugin_dir_path( __FILE__ ) . 'blocks/build/index.js' ) );
wp_register_script( 'wp-i18n', plugins_url( 'i18n/build/index.js', __FILE__ ), array(), gutenberg_asset_cache_version( 'i18n/build/index.js' ) );
wp_register_script( 'wp-element', plugins_url( 'element/build/index.js', __FILE__ ), array( 'react', 'react-dom', 'react-dom-server' ), gutenberg_asset_cache_version( 'element/build/index.js' ) );
wp_register_script( 'wp-blocks', plugins_url( 'blocks/build/index.js', __FILE__ ), array( 'wp-element', 'tinymce-nightly' ), gutenberg_asset_cache_version( 'blocks/build/index.js' ) );

// Editor Styles.
wp_register_style( 'wp-blocks', plugins_url( 'blocks/build/style.css', __FILE__ ), array(), filemtime( plugin_dir_path( __FILE__ ) . 'blocks/build/style.css' ) );
wp_register_style( 'wp-blocks', plugins_url( 'blocks/build/style.css', __FILE__ ), array(), gutenberg_asset_cache_version( 'blocks/build/style.css' ) );
}
add_action( 'init', 'gutenberg_register_scripts' );

Expand Down Expand Up @@ -159,7 +159,7 @@ function gutenberg_scripts_and_styles( $hook ) {
'wp-editor',
plugins_url( 'editor/build/index.js', __FILE__ ),
array( 'wp-i18n', 'wp-blocks', 'wp-element' ),
filemtime( plugin_dir_path( __FILE__ ) . 'editor/build/index.js' ),
gutenberg_asset_cache_version( 'editor/build/index.js' ),
true // $in_footer
);

Expand Down Expand Up @@ -213,11 +213,44 @@ function gutenberg_scripts_and_styles( $hook ) {
'wp-editor',
plugins_url( 'editor/build/style.css', __FILE__ ),
array( 'wp-blocks' ),
filemtime( plugin_dir_path( __FILE__ ) . 'editor/build/style.css' )
gutenberg_asset_cache_version( 'editor/build/style.css' )
);
}
add_action( 'admin_enqueue_scripts', 'gutenberg_scripts_and_styles' );

/**
* Set's a script/style version for cache busting.
*
* 0.1.0
*
* @param string $file The file path relative to the plugin's root.
*
* @return string
*/
function gutenberg_asset_cache_version( $file = null ) {
$version = false;

if ( defined( 'WP_DEBUG' ) && WP_DEBUG && $file ) {
if ( file_exists( plugin_dir_path( __FILE__ ) . $file ) ) {
Copy link
Member

@aduth aduth May 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice addition of the file_exists. I think I'd seen some discussion where this was previously logging a warning if the file didn't exist.

$version = filemtime( plugin_dir_path( __FILE__ ) . $file );
}
} else {
$version = get_transient( 'gutenberg_version' );

if ( ! $version && function_exists( 'get_plugin_data' ) ) {
$plugin = get_plugin_data( __FILE__, false );

if ( ! empty( $plugin['Version'] ) ) {
$version = $plugin['Version'];
}

set_transient( 'gutenberg_version', $version, DAY_IN_SECONDS );
Copy link
Member

@aduth aduth May 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the logic here, if the version is not assigned, we'll perform a get_transient and set_transient on every request. I think we'll either want to:

  • Only set_transient if Version is not empty (move into condition above)
  • Allow transient to be set to an empty string, but strictly check this before calling get_plugin_data

My preference is toward the second, which is a matter of changing ! $version to false === $version to reflect we only want the condition to match if get_transient returns it's empty state, but not for the empty string.

If the transient does not exist, does not have a value, or has expired, then get_transient will return false.

https://codex.wordpress.org/Function_Reference/get_transient

Practically speaking, we know that the version will be assigned (it already is), but if we were so confident, we'd also not need the ! empty( $plugin['Version'] ) check. So one way or another, we should be consistent.

Copy link
Member Author

@kopepasah kopepasah May 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree the second option is probably best and being more explicit by using false === $version is a good idea; however, allowing an empty value (while it may never be empty in this case) would not be ideal if we want the transient to update if the value were to return not empty (after set as empty).

How about something like:

if ( false === $version && function_exists( 'get_plugin_data' ) ) {
	$plugin = get_plugin_data( __FILE__, false );

	if ( ! empty( $plugin['Version'] ) ) {
		$version = $plugin['Version'];

		set_transient( 'gutenberg_version', $version, DAY_IN_SECONDS );
	}
}

}
}

return $version;
}

/**
* Load plugin text domain for translations.
*
Expand Down