asset view helper BC about assetmanager HOT 10 CLOSED

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

from assetmanager.

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.

from assetmanager.

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

from assetmanager.

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

from assetmanager.

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.

from assetmanager.

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
        ]
    ],
];

from assetmanager.

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

from assetmanager.

@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?

from assetmanager.

@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.

from assetmanager.

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

from assetmanager.