Hey Wesley, cc @Ocramius
Check out this failing test: https://github.com/solcre/AssetManager/blob/bugfix/duplicate-filters/tests/AssetManagerTest/Service/AssetManagerTest.php
Here is what I told you about an AssetCollection having filers duplicated. This test is a little bit messy, but it was a bit hard to replicate the scenario. Also i'm not sure if this test should be in AssetManagerTest or if in another test class.
What this test shows is that a filter (ReverseFilter) is dumped twice because it is ensured once for the asset (asset-path.js, the mocked one) and once for the AssetCollection (blah.js) that contains the asset.
Assetic verifies that this won't happen in FilterCollection.
The problem is in AssetFilterManager, if the filter is passed as string, it will get instanced everytime ensureByFilter is called.
My fix suggests to keep an array of already instanced filters, and when ensureByFilter is called checks if it is already instanced then return the instance. This way Assetic will detect that the filters are the same in the FilterCollection and assets won't dump filters twice.
If you want to instance a new filter for each asset, you can still pass the instance to AssetFilterMangager (line 128) or you can use ensureByService.
What do you think?
Hey Wesley, cc @Ocramius
Check out this failing test: https://github.com/solcre/AssetManager/blob/bugfix/duplicate-filters/tests/AssetManagerTest/Service/AssetManagerTest.php
Here is what I told you about an AssetCollection having filers duplicated. This test is a little bit messy, but it was a bit hard to replicate the scenario. Also i'm not sure if this test should be in AssetManagerTest or if in another test class.
What this test shows is that a filter (ReverseFilter) is dumped twice because it is ensured once for the asset (asset-path.js, the mocked one) and once for the AssetCollection (blah.js) that contains the asset.
Assetic verifies that this won't happen in FilterCollection.
The problem is in AssetFilterManager, if the filter is passed as string, it will get instanced everytime ensureByFilter is called.
My fix suggests to keep an array of already instanced filters, and when ensureByFilter is called checks if it is already instanced then return the instance. This way Assetic will detect that the filters are the same in the FilterCollection and assets won't dump filters twice.
If you want to instance a new filter for each asset, you can still pass the instance to AssetFilterMangager (line 128) or you can use ensureByService.
What do you think?