asset view helper BC

[thomasvargiu]

215 overwrite the Zend\View\Helper\Asset view helper alias, so it's a break compatibility. At least you should document it.

[RWOverdijk]

I'm sorry, I don't understand. It's an alias, does it break something?

[thomasvargiu]

I should see it better on zend-view, but I think aliaseshas precedences on factories. zend-view defines an asset alias, so before this release it took precedence on AssetManager asset factory.

Now you are defining an alias and it's overwritting the zend-view alias.

Now I have to re-define the zend-view alias on application level config if I want to use it.

[RWOverdijk]

@FabianKoestring @wshafer this sounds like a valid point. ☝️

[FabianKoestring]

I reused the key asset. Didn´t know Zend has an own asset Helper. @RWOverdijk - Maybe change Alias?

[RWOverdijk]

Maybe, but it does sound like another BC break which... I'm not a huge fan of. Especially since I'm in maintainer mode.

I'd like for you three to collectively advise on what to do next, would that be possible? Maybe I can pull in my favourite sceptic @Ocramius if he has time.

[thomasvargiu]

I think it's not possibile to avoid a BC. If you think about it, the asset helper of this library became broken since Zend released the asset view helper. Overriding the Zend helper is not a good think. I suggest to leave the asset name in factories (for BC), adding the class name too, but then adding another dedicate alias for this package. Your asset helper will become deprecated (still broken if someone are using a new zend-view release), and the new helper will be something like assetManager.

TL;TR Suggested change is:

return [
    'view_helpers'    => [
        'factories' => [
            'asset' => AssetManager\Service\AssetViewHelperFactory::class,
            AssetManager\View\Helper\Asset::class => AssetManager\Service\AssetViewHelperFactory::class
        ],
        'aliases' => [
            'assetManager' => AssetManager\View\Helper\Asset::class
        ]
    ],
];

[RWOverdijk]

Or we revert the change and move it to a major bump with the suggested change. But that's up to @wshafer because I don't know the state of things right now

[wshafer]

@RWOverdijk - Sorry for the delay. I've been trying to wrap my head around this package in general. I was planning on releasing the new version until I noticed a problem upstream. It appears that our upstream package of Assetic is no longer being maintained. That means that no matter what we do with this package, the project as a whole is stale.

At this point, I'm thinking perhaps we allow this change and bump the version number so it continues to work for awhile. But I also think we should also mark this as a deprecated package so we don't encourage others to continue using this and instead find other solutions to the asset problem. With no Assetic there's no Asset Manager package either. And I certainly don't have time to manage that upstream project, I suspect you don't either.

Thoughts?

[RWOverdijk]

@wshafer Your suspicion is correct. In general I don't think this module is the best solution anymore.

I think the module can be marked as deprecated. However, assetic is pretty stable in its current state and I'm still not against merging PRs on this library. So it's not "dead", just potentially dying. A clear note on this in the readme has my preference.

[wshafer]

I'm good with that. I'll mark my forks with the same notice.