[AssetMapper] Truncate public digests to 8 characters #57879
Conversation
src/Symfony/Component/AssetMapper/Tests/Factory/MappedAssetFactoryTest.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/AssetMapper/Factory/MappedAssetFactory.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/AssetMapper/Factory/MappedAssetFactory.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/AssetMapper/Factory/MappedAssetFactory.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/AssetMapper/Tests/Factory/MappedAssetFactoryTest.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/AssetMapper/Tests/Factory/MappedAssetFactoryTest.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/AssetMapper/Tests/Factory/MappedAssetFactoryTest.php
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
If the goal is to generate shorter hashes, then changing the algorithm is not going to be the right solution IMHO.
Using base64 (or base58) would have the most substantial impact to me.
And if this not enough, truncating the hash.
Considering these, the most flexible option would be to accept a closure instead. This doesn't require a full abstraction (aka interface) while still providing all the needed flexibility.
But are we sure we want to make this configurable? Can't we update the hash-generation logic so that everybody will get a shorter hash without being exposed to too-short hashes?
|
You're right, this implementation is in fact not required to do what i initially wanted. I think i can find another method via the "public path" & the asset package... to keep unique digest locally while exposing shorter ones. I'll rework this later today |
|
Shorter hashes for everyone 👌🏻 |
|
Webpack seems to have two options I don't think there would be any problem if we allow to reduce digests down to 7 caracters (the risk of collision for one given file is practically null). So instead of configure the hash, could we add an option "digestLength" / "hashLength" ? @nicolas-grekas with this "hashLenght" idea, would a closure still be necessary ? I'd rather avoid it, as it would create difficulties: we need to enforce some things currently to extract/identify the digest part in file names. |
|
Why make it configurable at all ? Base64+7chars and done, no ? |
|
I agree with Nicolas and I don't think we should make this configurable ... but if we think we can change this for all to improve it, let's change it. Thanks. |
|
Let's shorten the public digest for everyone then :) @nicolas-grekas i suggest we keep only hexa characters for now, as it's safer to catch (in dev) or identifiy (i'm thinking web servers or CDN using url patterns, log exclusion, ...) .. wdyt ? If we truncate down to 7 caracters we keep the same "contract" than before. (PR update incoming) |
|
PR Updated I reverted what i did before (with hash algorithm) It now only truncates the public digest down to 8 caracters (8 feels more similar to what can be found in gulp/webpack/etc. and still way shorter than today) This solution is fully compatible with the current behaviour and requires no change on config/framework/recipe/ or user code. Thanks everyone for the feedback / ideas -- PHP 8.2 high-deps failures unrelated (doctrine) |
|
I'd do the base64 now because that allows a lot more entropy per character. We could then truncate to 7 bytes. |
|
@nicolas-grekas added base64 encode & now truncate to 7 ! -- PHP 8.4 failures unrelated (mime) |
3fa211d to
e1ae985
Compare
|
Thank you @smnandre. |
This pull request (PR) now shortens the public digest of mapped assets to 8 characters.
The digest is used in the filenames of deployed assets, leading to a tangible reduction in HTML size, particularly in the generated import map.
--
Original message:
I've been wanting to use shorter digests in my mapped assets / importmap URL's.
This PR adds an hashAlgorithm argument to the MappedAssetFactory to do so.
I'm not sure this is enough/good choice, as we need some requirement regarding the digests (7+ caracters, alnum only)
But adding an entire AssetHasherInterface would be probably overkill, so i'm very open for feedback / ideas there.
(I did not put FrameworkBundle modifications in this PR, as the discussion will probably have an impact on what's to do)