Skip to content

[AssetMapper] add support for assets pre-compression #59020

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

Jump to bottom
Merged
merged 1 commit into from
Dec 6, 2024
Merged

[AssetMapper] add support for assets pre-compression #59020

merged 1 commit into from
Dec 6, 2024

Conversation

dunglas
Copy link
Member

@dunglas dunglas commented Nov 27, 2024 •
edited by chalasr
Loading

Q A
Branch? 7.3
Bug fix? no
New feature? yes
Deprecations? no
Issues n/a
License MIT

Adds support for assets and files precompression using Zstandard, Brotli, and gzip (this will be part of my talk at SymfonyCon next week).

When calling bin/console asset-map:compile and if the new option asset_mapper.precompress is enabled, all files matching the configured extensions will be compressed in Brotli, Zstandard, and gzip (zopfli when available) at maximal compression (defaults to the list supported by Caddy and Cloudflare). The PHP extension is used if available, otherwise the Unix command is used as a fallback.

The compressed files are created with the same name as the original asset but with the .br, .zst, or .gz extension appended.
This allows native compatibility with web servers supporting precompression such as Caddy and FrankenPHP or NGINX with the Brotli module.
Apache can also use these files but will require some extra config.

This PR also adds an assets:compress command that can be used to compress files not managed by AssetMapper (e.g. robots.txt).

Finally, the new asset_mapper.compressor service can be used to precompress files uploaded by the user (among various other use cases).

TODO:

  • add tests

@carsonbot carsonbot added this to the 7.3 milestone Nov 27, 2024
@dunglas dunglas force-pushed the asset-mapper-precompress branch 2 times, most recently from 287ee79 to b268420 Compare November 27, 2024 16:16
@dunglas
Copy link
Member Author

dunglas commented Nov 27, 2024

The Psalm errors are false positives (the constants are defined if the extensions are installed). Should I ignore them explicitly or let the baseline generator do is job?

@smnandre
Copy link
Member

Thinking out loud here... I'm wondering if this could/should be in a dedicated package ?

Except the "compress on write", the Compressor is not really linked to the AssetMapper.. and maybe could benefit other components / projects ?

@dunglas
Copy link
Member Author

dunglas commented Nov 27, 2024

To be honest, I hesitated between putting it in Assets (but that's not its responsibility at all), AssetMapper or in its own component. But I thought it was too small to be in a stand-alone component.

I've no strong opinion about this. Let's wait for @symfony/mergers opinions.

@dunglas dunglas force-pushed the asset-mapper-precompress branch from e863ee2 to 4bbfe60 Compare November 28, 2024 13:26
@stof
Copy link
Member

stof commented Nov 29, 2024

The Psalm errors are false positives (the constants are defined if the extensions are installed). Should I ignore them explicitly or let the baseline generator do is job?

Let the baseline generator do its job for next PRs (ignoring the job failure in this PR). We don't add explicit Psalm ignore rules in the project

@dunglas dunglas force-pushed the asset-mapper-precompress branch from f1f4b27 to b64d3df Compare December 1, 2024 11:39
@dunglas dunglas mentioned this pull request Dec 1, 2024
Open
@dunglas dunglas force-pushed the asset-mapper-precompress branch 2 times, most recently from 4895f54 to f0e62b4 Compare December 2, 2024 20:46
chalasr
chalasr approved these changes Dec 3, 2024
View reviewed changes
Copy link
Member

@chalasr chalasr left a comment

Choose a reason for hiding this comment

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

With minor comment

@fabpot fabpot force-pushed the asset-mapper-precompress branch from b02b01a to 5fa3e92 Compare December 6, 2024 13:38
@fabpot
Copy link
Member

fabpot commented Dec 6, 2024

Thank you @dunglas.

@fabpot fabpot merged commit 12e4e53 into symfony:7.3 Dec 6, 2024
3 of 5 checks passed
@fabpot fabpot mentioned this pull request Dec 6, 2024
Closed
stof
stof reviewed Dec 7, 2024
View reviewed changes
src/Symfony/Component/AssetMapper/Compressor/ZopfliCompressor.php
(new Process([$this->executable, '--', $path]))->mustRun();
}

private function createStreamContext()
Copy link
Member

Choose a reason for hiding this comment

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

@dunglas this method with no return type and no phpdoc @return makes our CI fail for the check about our migration to native return types.

Copy link
Member Author

Choose a reason for hiding this comment

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

The type is documented in the PHPDoc of the abstract function in the trait, but I can add it here too if needed.

src/Symfony/Component/AssetMapper/Compressor/GzipCompressor.php
final class GzipCompressor implements SupportedCompressorInterface
{
use CompressorTrait {
compress as baseCompress;
Copy link
Member

Choose a reason for hiding this comment

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

This should be used as private baseCompress instead. We don't want to expose a public method.

src/Symfony/Component/AssetMapper/Compressor/GzipCompressor.php

public function compress(string $path): void
{
if (null === $reason = $this->zopfliCompressor->getUnsupportedReason()) {
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't the getUnsupportedReason implementation also check whether zopfli is supported ? Maybe your server has the zopfli executable but does not have the gzip one and the zlib extension. In such case, the current implementation would mark this compressor as unsupported while the compress method would work

@dunglas dunglas mentioned this pull request Dec 7, 2024
Merged
fabpot added a commit that referenced this pull request Dec 9, 2024
@fabpot
bug #59130 [AssetMapper] minor fixes for pre-compression (dunglas)
0a44b87 
This PR was merged into the 7.3 branch.

Discussion
----------

[AssetMapper] minor fixes for pre-compression

| Q             | A
| ------------- | ---
| Branch?       | 7.3
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Issues        | #59020 (review)
| License       | MIT

Fixes `@stof`'s comments.

Commits
-------

b6597b6 [AssetMapper] minor fixes for pre-compression
@alexander-schranz
Copy link
Contributor

Nice work 👍 , I think its worth mentioning that nginx gzip with gzip_static on which is enable in most Distributions can also be used with this new feature not only the brotli extension: https://nginx.org/en/docs/http/ngx_http_gzip_static_module.html

@fabpot fabpot mentioned this pull request May 2, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stof stof stof left review comments

@smnandre smnandre smnandre left review comments

@tinect tinect tinect requested changes

@fabpot fabpot fabpot approved these changes

@mykiwi mykiwi mykiwi approved these changes

@GromNaN GromNaN GromNaN approved these changes

@mtarld mtarld mtarld approved these changes

@chalasr chalasr chalasr approved these changes

Assignees
No one assigned
Labels
AssetMapper Feature Status: Reviewed
Projects
None yet
Milestone
7.3
Development

Successfully merging this pull request may close these issues.