Make WordPress Core

Opened 6 months ago

Closed 5 weeks ago

#61485 closed enhancement (fixed)

Refactor Etag-based cache busting for styles and scripts solution and add tests

Reported by: vrajadas's profile vrajadas Owned by: peterwilsoncc's profile peterwilsoncc
Milestone: 6.7 Priority: normal
Severity: normal Version:
Component: Script Loader Keywords: has-patch has-unit-tests commit dev-reviewed
Focuses: Cc:

Description

Following issue #58433, I wanted to add tests to the solution and then realised they is code duplication. I move the logic to method in the class both wp_script and wp_style extend.

I also added tests.
Test instructions are the same as in #58433, since this issue doesn't change functionality.
Github PR is https://github.com/WordPress/wordpress-develop/pull/6820

Change History (12)

This ticket was mentioned in PR #6820 on WordPress/wordpress-develop by @vrajadas.


6 months ago
#1

  • Keywords has-patch has-unit-tests added

Added tests to change make in PR for 58433

Moved the logic to a separate method under wp-dependencies class and added unit tests.

Trac ticket: https://core.trac.wordpress.org/ticket/61485

@vrajadas commented on PR #6820:


6 months ago
#2

Hey @peterwilsoncc I created a new trac ticket and updated the PR description with it's new number.

@martin.krcho commented on PR #6820:


5 months ago
#3

@mukeshpanchal27 would you please re-review this task?

#4 @mukesh27
5 months ago

  • Milestone changed from Awaiting Review to 6.7
  • Version 6.6 deleted

The PR looks good to me and approved, happy to get more folks eye.

Move to 6.7 milestone.

#5 @peterwilsoncc
4 months ago

  • Owner set to peterwilsoncc
  • Resolution set to fixed
  • Status changed from new to closed

In 58935:

Script Loader: Refactor Etag generation for concatenated assets.

Move Etag HTTP header generation in load-scripts.php and load-styles.php to WP_Dependencies.

Introduces the method WP_Dependencies::get_etag() and associated unit tests.

Follow up to [57943].

Props vrajadas, martinkrcho, mukesh27.
Fixes #61485.

#7 @swissspidy
5 weeks ago

Bug identified that was introduced here: https://github.com/WordPress/wordpress-develop/pull/7715

cc @peterwilsoncc

#8 @SergeyBiryukov
5 weeks ago

In 59341:

Script Loader: Correct the number of arguments passed to WP_Styles::get_etag().

This fixes an issue with the usage of the new $wp_styles->get_etag() method in wp-admin/load-styles.php, where $wp_version is passed as the first argument instead of $load being used as the only argument.

Follow-up to [58935].

Props justlevine, mukesh27, swissspidy.
See #52217, #61485.

#9 @SergeyBiryukov
5 weeks ago

  • Keywords commit dev-feedback added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for merging [59341] to the 6.7 branch after a second committer's review.

#10 @swissspidy
5 weeks ago

  • Keywords dev-reviewed added; dev-feedback removed

#11 @peterwilsoncc
5 weeks ago

In 59343:

Script Loader: Correct the number of arguments passed to WP_Styles::get_etag().

This fixes an issue with the usage of the new $wp_styles->get_etag() method in wp-admin/load-styles.php, where $wp_version is passed as the first argument instead of $load being used as the only argument.

Follow-up to [58935].

Reviewed by swissspidy.
Merges [59341] to the 6.7 branch.

Props justlevine, mukesh27, swissspidy, SergeyBiryukov.
See #52217, #61485.

#12 @desrosj
5 weeks ago

  • Resolution set to fixed
  • Status changed from reopened to closed

This seems to have been appropriately addressed, the commit message just lacked a Fixed line.

Closing out, but please reopen if I'm missing something.

Note: See TracTickets for help on using tickets.