Add asset cache version helper function. #585

kopepasah · 2017-05-01T11:50:37Z

This helper function will use filemtime while using WP_DEBUG for development and Gutenberg's version for package releases.

aduth · 2017-05-01T12:35:29Z

Remarking from #440 as it relates to the changes here:

One final consideration is should we implement the filemtime with WP_DEBUG or SCRIPT_DEBUG/STYLE_DEBUG?

Per documentation, it sounds like SCRIPT_DEBUG might be the more appropriate (or more specific anyways) constant.

aduth · 2017-05-01T12:38:45Z

index.php

+	$version = false;
+
+	if ( defined( 'WP_DEBUG' ) && WP_DEBUG && $file ) {
+		if ( file_exists( plugin_dir_path( __FILE__ ) . $file ) ) {


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.

aduth · 2017-05-01T12:40:12Z

index.php

+					$version = $plugin['Version'];
+				}
+			} else {
+				$version = false;


It doesn't seem to follow that $value could be anything other than $false at this point (except manual tampering of the transient). Do we need to assign this?

And if we do eliminate the else, we could probably collapse the function_exists check into the ! $version condition to bump down the indentation one.

You are correct. Originally I had set the transient outside the brackets and forgot to remove after moving.

aduth · 2017-05-01T12:58:26Z

index.php

+			if ( function_exists( 'get_plugin_data' ) ) {
+				$plugin = get_plugin_data( __FILE__, false );
+
+				if ( isset( $plugin['Version'] ) ) {


In my testing, it appears the Version key will always be set in the return value from get_plugin_data, but could be empty. Therefore we'll probably want ! empty instead of isset here.

! empty sound like a good addition.

kopepasah · 2017-05-01T13:23:36Z

@aduth I pushed up a few changes to address your comments.

aduth · 2017-05-01T13:33:15Z

@kopepasah I'm not seeing any changes. Did you forget to push?

aduth · 2017-05-01T13:54:35Z

index.php

+				$version = $plugin['Version'];
+			}
+
+			set_transient( 'gutenberg_version', $version, DAY_IN_SECONDS );


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.

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 ); } }

nylen · 2017-05-01T15:18:27Z

How do we ensure that the stored version transient will be invalidated on plugin upgrade (or install/reinstall)? I think that needs to be done here.

aduth · 2017-05-01T23:39:43Z

How do we ensure that the stored version transient will be invalidated on plugin upgrade (or install/reinstall)? I think that needs to be done here.

At that point, I wonder if it's all worth pulling, caching, and invalidating the plugin metadata vs. just define-ing a constant, even if it's duplicated.

kopepasah · 2017-05-02T02:59:13Z

How do we ensure that the stored version transient will be invalidated on plugin upgrade (or install/reinstall)?

We should implement activate/deactivate hooks to set the plugin version.

At that point, I wonder if it's all worth pulling, caching, and invalidating the plugin metadata vs. just define-ing a constant, even if it's duplicated.

While I typically do not like defining constants for version numbers, I agree it might be worth considering in this case.

westonruter · 2017-05-02T22:40:19Z

We should implement activate/deactivate hooks to set the plugin version.

Such hooks can be considered harmful. See WordPress/WordPress-Coding-Standards#881 (comment)

kopepasah · 2017-05-03T03:36:48Z

@westonruter thanks for pointing that out.

At this point I would say it is probably best to use a constant for the version number.

nylen · 2017-06-12T19:45:47Z

At this point I would say it is probably best to use a constant for the version number.

This was implemented in #1025 as part of the plugin zip build process. This PR can be simplified significantly as a result: use the GUTENBERG_VERSION constant if it is defined, otherwise use filemtime.

gziolo

There was no activity for a quite some time and now the changed file is conflicting. @kopepasah, do you plan to resolve those conflicts and address all above comments?

nylen · 2017-09-23T04:17:15Z

This was superseded by #1025.

nylen · 2017-09-23T04:18:04Z

Never mind. I should have read the comments including my latest more carefully :)

kopepasah · 2017-09-23T05:38:12Z

Sorry, have been away. I can resolve these changes in the next 24 hours.

# Conflicts: # index.php

kopepasah · 2017-09-23T15:35:03Z

After review, I can see now that filemtime is used in many areas for enqueuing scripts from client-assets.php. In addition, there is information in docs/blocks-stylesheet.md which mentions using filemtime as how to add the version of the stylesheet.

Before I make all of these updates, are we certain we want to use GUTENBERG_VERSION as the scripts and stylesheet version?

Not familiar with this part of Gutenberg.

gziolo · 2017-09-25T10:18:33Z

@pento can you help with a review here or help find someone else?

kopepasah · 2017-09-25T16:16:01Z

@pento I am happy to make the changes, but want to verify just to make sure we are going this route before spending the time.

codecov · 2017-09-25T16:43:43Z

Codecov Report

Merging #585 into master will increase coverage by 0.03%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #585      +/-   ##
==========================================
+ Coverage   33.69%   33.72%   +0.03%     
==========================================
  Files         185      191       +6     
  Lines        5594     5903     +309     
  Branches      976     1043      +67     
==========================================
+ Hits         1885     1991     +106     
- Misses       3141     3308     +167     
- Partials      568      604      +36

Impacted Files	Coverage Δ
editor/sidebar/post-schedule/index.js	`63.15% <0%> (-1.85%)`	⬇️
editor/inserter/index.js	`0% <0%> (ø)`	⬆️
editor/modes/visual-editor/index.js	`0% <0%> (ø)`	⬆️
editor/modes/visual-editor/block-list.js	`0% <0%> (ø)`	⬆️
editor/document-outline/item.js	`0% <0%> (ø)`
editor/document-outline/index.js	`0% <0%> (ø)`
editor/sidebar/document-outline-panel/index.js	`0% <0%> (ø)`
editor/modes/visual-editor/inserter.js	`100% <0%> (ø)`
editor/table-of-contents/index.js	`0% <0%> (ø)`
editor/word-count/index.js	`0% <0%> (ø)`
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 26369c3...4714c93. Read the comment docs.

pento · 2017-09-26T00:57:37Z

So, GUTENBERG_VERSION would be the ideal option for this, but it seems like it's only available in the release version of Gutenberg, which isn't so ideal. :-)

I'm pondering if we should modify gutenberg.php to set the version to the current git commit, that'll be replaced with the proper version number during the release process. Ie, something like:

define( 'GUTENBERG_VERSION', trim( shell_exec( 'git rev-parse HEAD' ) ) );

nylen · 2017-09-26T02:49:36Z

Wouldn't it be better to use a function which returns the mtime in development mode? Otherwise things may be cached in between commits.

pento · 2017-09-26T03:03:19Z

A function seems like a bit of overkill, and I'm not wild about the pattern of writing out the filename twice.

Also, if you're actively editing code, your dev environment should either be not caching, or responding with an appropriate ETag or Last-Modified header for cache invalidation.

nylen · 2017-09-26T15:22:29Z

your dev environment should either be not caching, or responding with an appropriate ETag or Last-Modified header for cache invalidation.

The reasoning behind using filemtime is that in practice this will often not be the case, and we should ensure that dev environments behave predictably anyway.

Edit: The above is just to explain the reasoning behind the existing decision. I won't be working on this further and don't have a strong opinion about the best way forward.

staylor · 2017-11-03T16:21:29Z

This PR has zero changes - close?

kopepasah added 2 commits May 1, 2017 20:47

440 Add asset cache version helper function.

a7a540c

Merge branch 'master' into add/440-script-versioning-cachebust

8642ea1

kopepasah mentioned this pull request May 1, 2017

Use get_plugin_data for production script versioning (cachebust) #440

Closed

aduth reviewed May 1, 2017

View reviewed changes

440 Simplify conditional checks and use empty check.

4a67289

aduth reviewed May 1, 2017

View reviewed changes

mtias added the Framework Issues related to broader framework topics, especially as it relates to javascript label May 3, 2017

nylen mentioned this pull request Jun 9, 2017

Do not download vendor JS files in released plugin versions #1025

Merged

gziolo previously requested changes Sep 22, 2017

View reviewed changes

nylen closed this Sep 23, 2017

nylen reopened this Sep 23, 2017

Merge branch 'master' into add/440-script-versioning-cachebust

4714c93

# Conflicts: # index.php

gziolo requested a review from pento September 25, 2017 10:18

danielbachhuber closed this Jan 23, 2018

danielbachhuber deleted the add/440-script-versioning-cachebust branch January 23, 2018 15:55

naiba4 mentioned this pull request Jun 20, 2023

[Snyk] Fix for 1 vulnerabilities naiba4/gutenberg#10

Open

yassine4999 mentioned this pull request Jun 20, 2023

[Snyk] Fix for 1 vulnerabilities yassine4999/gutenberg#13

Open

Verdinjoshua26 mentioned this pull request Jun 20, 2023

[Snyk] Fix for 1 vulnerabilities Verdinjoshua26/gutenberg#71

Open

monzerfoda mentioned this pull request Jun 21, 2023

[Snyk] Fix for 1 vulnerabilities monzerfoda/gutenberg#12

Open

MaxMood96 mentioned this pull request Jun 22, 2023

[Snyk] Fix for 1 vulnerabilities MaxMood96/gutenberg#100

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add asset cache version helper function. #585

Add asset cache version helper function. #585

kopepasah commented May 1, 2017

aduth commented May 1, 2017

aduth May 1, 2017

aduth May 1, 2017

aduth May 1, 2017

kopepasah May 1, 2017

aduth May 1, 2017

kopepasah May 1, 2017

kopepasah commented May 1, 2017

aduth commented May 1, 2017

aduth May 1, 2017

kopepasah May 1, 2017

nylen commented May 1, 2017

aduth commented May 1, 2017

kopepasah commented May 2, 2017

westonruter commented May 2, 2017

kopepasah commented May 3, 2017

nylen commented Jun 12, 2017

gziolo left a comment

nylen commented Sep 23, 2017

nylen commented Sep 23, 2017

kopepasah commented Sep 23, 2017

kopepasah commented Sep 23, 2017

gziolo commented Sep 25, 2017

kopepasah commented Sep 25, 2017

codecov bot commented Sep 25, 2017

pento commented Sep 26, 2017

nylen commented Sep 26, 2017

pento commented Sep 26, 2017

nylen commented Sep 26, 2017 •

edited

Loading

staylor commented Nov 3, 2017

Add asset cache version helper function. #585

Add asset cache version helper function. #585

Conversation

kopepasah commented May 1, 2017

aduth commented May 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kopepasah commented May 1, 2017

aduth commented May 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nylen commented May 1, 2017

aduth commented May 1, 2017

kopepasah commented May 2, 2017

westonruter commented May 2, 2017

kopepasah commented May 3, 2017

nylen commented Jun 12, 2017

gziolo left a comment

Choose a reason for hiding this comment

nylen commented Sep 23, 2017

nylen commented Sep 23, 2017

kopepasah commented Sep 23, 2017

kopepasah commented Sep 23, 2017

gziolo commented Sep 25, 2017

kopepasah commented Sep 25, 2017

codecov bot commented Sep 25, 2017

Codecov Report

pento commented Sep 26, 2017

nylen commented Sep 26, 2017

pento commented Sep 26, 2017

nylen commented Sep 26, 2017 • edited Loading

staylor commented Nov 3, 2017

nylen commented Sep 26, 2017 •

edited

Loading