Skip to content

Add AssetMapper support - #1645

Merged
dbu merged 22 commits into
liip:2.xfrom
tito10047:asset-mapper
Jan 5, 2026
Merged

Add AssetMapper support#1645
dbu merged 22 commits into
liip:2.xfrom
tito10047:asset-mapper

Conversation

@tito10047

@tito10047 tito10047 commented Dec 23, 2025

Copy link
Copy Markdown
Contributor
Q A
Branch? 2.x
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Fixed tickets #1608
License MIT
Doc

This pr introduce asset_mapper integration. its created assetMaper locator that is injected to filesystem loader.

when@dev:
    liip_imagine:
        loaders:
            asset_mapper:
                asset_mapper:
            chain:
                chain:
                    loaders: [ asset_mapper, default ]
        data_loader: chain

@tito10047 tito10047 changed the title Added AssetMapperLoaderFactory and AssetMapperLocator Add AssetMapper support Dec 23, 2025
@tito10047
tito10047 marked this pull request as ready for review December 25, 2025 06:56
@tito10047

Copy link
Copy Markdown
Contributor Author

Merry Christmas :)

@dbu dbu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

thanks a lot for looking into this!

seems like a good addition, and seems to work with little additional code 👍

i have a few questions and suggestions

Comment thread Resources/config/imagine.php Outdated
Comment thread Binary/Locator/AssetMapperLocator.php
Comment thread Binary/Locator/AssetMapperLocator.php
@dbu

dbu commented Jan 5, 2026

Copy link
Copy Markdown
Collaborator

i think you need to add the asset mapper to the dev dependencies in composer.json

not sure what is going on with the legacy builds, those failures are probably unrelated to your changes.

@dbu

dbu commented Jan 5, 2026

Copy link
Copy Markdown
Collaborator

not sure what is going on with the legacy builds, those failures are probably unrelated to your changes.

actually, this is about using language features that are only available in php 8. but i think its time to ditch that ultra legacy. i will adjust the minimum requirement to php 8.

@dbu

dbu commented Jan 5, 2026

Copy link
Copy Markdown
Collaborator

please rebase on the 2.x branch to build with only php 8, that should fix the build errors

…rt caching, updated PHPDoc, and standardized configuration formatting
@coveralls

coveralls commented Jan 5, 2026

Copy link
Copy Markdown

Coverage Status

coverage: 79.645% (-0.6%) from 80.201%
when pulling ae06e8f on tito10047:asset-mapper
into dec6314 on liip:2.x.

Comment thread Binary/Locator/AssetMapperLocator.php Outdated
Comment thread Binary/Locator/AssetMapperLocator.php Outdated

@dbu dbu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

thanks, i think we are almost there.

can you please look at the cs fixer report? and i commented a few last things.

Comment thread Resources/config/imagine.php Outdated
Comment thread Resources/doc/data-loader/asset_mapper.rst Outdated
Comment thread Resources/doc/data-loader/asset_mapper.rst Outdated
Comment thread DependencyInjection/Factory/Loader/AssetMapperLoaderFactory.php Outdated
@tito10047

Copy link
Copy Markdown
Contributor Author

I dont know where to fix Coverrals. there must be added asset mapper package when symhony is higger than 6.4

Comment thread .github/workflows/phpstan.yml Outdated
Comment thread .github/workflows/phpunit.yml
tito10047 and others added 3 commits January 5, 2026 14:41
Co-authored-by: David Buchmann <david@liip.ch>
Co-authored-by: David Buchmann <david@liip.ch>

@dbu dbu left a comment

Copy link
Copy Markdown
Collaborator

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!

@dbu
dbu merged commit 9fe7089 into liip:2.x Jan 5, 2026
25 checks passed
@tito10047

Copy link
Copy Markdown
Contributor Author

thanks a lot for this contribution!

Can I help with version 3 ? is there something to do?

@dbu

dbu commented Jan 5, 2026

Copy link
Copy Markdown
Collaborator

released your changes as https://github.com/liip/LiipImagineBundle/releases/tag/2.17.0

for version 3: difficult question. we discussed the vision of radically refactoring the bundle to make the naming clearer and be able to better cover the various needs (e.g. on-the-fly image transform vs upfront transform). and get rid of most of the loader stuff in favor of flysystem (which did not exist when LiipImagineBundle was created, but now that it does there is no point in us maintaining e.g. S3 support in this bundle)
that was 1.5 years ago, and after initial brainstorming we did not find time to progress. as we want to really rethink the bundle, we need a focussed effort first to set the new basis... if it does sound interesting to you, a review of #1590 would be appreciated.

'', // will be injected by FilesystemLoaderFactory
])
->tag('liip_imagine.binary.locator', ['shared' => false]);
$services->set('liip_imagine.binary.locator.asset_mapper', AssetMapperLocator::class)

@dmaicher dmaicher Jan 5, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

we should mark this ->abstract() (it would also fix #1648)

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.

4 participants