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

Add support for JsonManifestVersionStrategy #1525

Closed

Conversation

wouterSkepp
Copy link

@wouterSkepp wouterSkepp commented Aug 28, 2023

Q A
Branch? 2.x
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Fixed tickets #1524
License MIT
Doc

This PR adds in support for JsonManifestVersionStrategy that is available in Symfony.
In a JSON manifest, we have a key => value structure where the key is the original filepath, and the value the versioned filestring, e.g:

{
  "build/images/cats.jpg": "/build/images/cats.jpg?v=dda78c62c1f2a8793334"
  "build/images/dogs.jpg": "/build/images/dogs.e6fbd6606990f549c912.jpg"
}

Similar to StaticVersionStrategy that loads it's configuration from the container, this reads the JSON Manifest file defined


framework:
    assets:
        json_manifest_path: '%kernel.project_dir%/public/build/manifest.json'
        

The LazyFilterRuntime has been extended to load the JSON Manifest file contents, and the various methods within to handle both Static & Json versioning.

Note that the current 2.x branch has unrelated failing CI.

I don't think this PR breaks current implementations although test coverage for AssetsVersionCompilerPass currently is lacking.

@coveralls
Copy link

Coverage Status

coverage: 81.851%. first build when pulling eb3132f on skepp:json_manifest_version_strategy_support into 3c0f20b on liip:2.x.

Copy link
Member

@dbu dbu left a comment

Choose a reason for hiding this comment

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

thanks a lot for this contribution! cool idea to support this feature.

i am not super sure if the path / resolvedPath are always the file path or if they can be the cache path as well.

DependencyInjection/Compiler/AssetsVersionCompilerPass.php Outdated Show resolved Hide resolved
DependencyInjection/Compiler/AssetsVersionCompilerPass.php Outdated Show resolved Hide resolved
DependencyInjection/Compiler/AssetsVersionCompilerPass.php Outdated Show resolved Hide resolved
Templating/LazyFilterRuntime.php Outdated Show resolved Hide resolved
return rtrim(mb_substr($path, 0, $start), '?');
}
} elseif ($this->jsonManifest) {
$asset = array_search($path, $this->jsonManifest, true);
Copy link
Member

Choose a reason for hiding this comment

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

searching a potentially long unsorted array is inefficient.

you could array_flip the array in the constructor and then do an array_key_exists and a simple key lookup instead for better performance. i guess both keys and values in json manifest should be unique.

} elseif ($this->jsonManifest) {
$asset = array_search($path, $this->jsonManifest, true);
if ($asset) {
return $asset;
Copy link
Member

Choose a reason for hiding this comment

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

can we be sure that the path from the manifest is right both for when the image needs to be generated and when it already exists?

Templating/LazyFilterRuntime.php Outdated Show resolved Hide resolved
@dbu
Copy link
Member

dbu commented Aug 30, 2023

i will fix the cs failures in the 2.x branch to be clean there.

@dbu
Copy link
Member

dbu commented Aug 30, 2023

i will fix the cs failures in the 2.x branch to be clean there.

the 2.x branch is green again. please rebase your branch on 2.x

@wouterSkepp
Copy link
Author

wouterSkepp commented Sep 4, 2023

Thank you for your helpful review! Updated PR accordingly.

i am not super sure if the path / resolvedPath are always the file path or if they can be the cache path as well.

I've not done a deep dive into the caching mechanism, but according to https://github.com/liip/LiipImagineBundle/blob/2.x/Resources/doc/asset-versioning.rst?plain=1#L10-L19
and from what I've seen so far is that this is always the original filepath.

@dbu
Copy link
Member

dbu commented Sep 7, 2023

i rebased and squashed the commits in #1529

@dbu dbu closed this Sep 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants