Skip to content
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.

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

Added filemtime to merged JS/CSS hash calculation algorithm #4004

Merged
merged 5 commits into from
May 29, 2024

Conversation

boesbo
Copy link
Contributor

@boesbo boesbo commented May 22, 2024

Description (*)

Add the timestamp of the files in the MD5 HASH generation criteria of the files.

Fixed Issues (if relevant)

  1. Fixes Improving the file name generation logic for JS and CSS files #4003

Questions or comments

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All automated tests passed successfully (all builds are green)
  • Add yourself to contributors list

@boesbo
Copy link
Contributor Author

boesbo commented May 28, 2024

If you can I ask you for a review @fballiano

@boesbo
Copy link
Contributor Author

boesbo commented May 28, 2024

@fballiano I improved the PR. There was some confusion with the variable name, so your question was legitimate. I have made the code clearer and accepted your suggestion not to run filemtime() several times;

@fballiano
Copy link
Contributor

@boesbo great, I'll test it out a bit

boesbo and others added 2 commits May 28, 2024 22:05
Co-authored-by: Fabrizio Balliano <fabrizio.balliano@gmail.com>
Co-authored-by: Fabrizio Balliano <fabrizio.balliano@gmail.com>
@boesbo boesbo requested a review from fballiano May 28, 2024 20:05
Copy link
Contributor

@fballiano fballiano left a comment

Choose a reason for hiding this comment

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

I don't like the way merging works by default in OM, but that's another topic, I've tested this PR and it does what it claims and I think it's ok to merge

@boesbo
Copy link
Contributor Author

boesbo commented May 28, 2024

Non mi piace il modo in cui funziona l'unione per impostazione predefinita in OM, ma questo è un altro argomento, ho testato questo PR e fa quello che afferma e penso che sia ok unire

It is only an improvement of the current functionality in view of a modern (now current) management of these assets in the browser cache. Thx!

@boesbo
Copy link
Contributor Author

boesbo commented May 28, 2024

If you can I ask you for a review @kasper-agg

@fballiano
Copy link
Contributor

I'll merge this in a couple of days if there's none against

@fballiano fballiano changed the title Last file timestamp added in JS and CSS merge Added filemtime to merged JS/CSS hash calculation algorithm May 29, 2024
@fballiano fballiano merged commit 1fcf745 into OpenMage:main May 29, 2024
17 checks passed
@kasper-agg
Copy link

If you can I ask you for a review @kasper-agg

What @fballiano already mentioned, the whole merging thing is another topic, your PR fixes the current issue at hand 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Core Relates to Mage_Core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improving the file name generation logic for JS and CSS files
4 participants