search

zendframework / zend-navigation

Navigation component from Zend Framework
BSD 3-Clause "New" or "Revised" License
20 stars 25 forks source link

Wrong method signatur in AbstractNavigationFactory #69

Closed DennisDobslaf closed 6 years ago

DennisDobslaf commented 6 years ago

https://github.com/zendframework/zend-navigation/blob/master/src/Service/AbstractNavigationFactory.php#L56

Method createService has a wrong signature implementing Zend\ServiceManager\FactoryInterface. It should be createService(ServiceLocatorInterface $serviceLocator); with just one parameter!

As of PHP 7.2 you'll receive a Fatal Error.

DennisDobslaf commented 6 years ago

Some additional Info:

I extend the AbstractNavigationFactory by myself, re-writing the createService Method (but following the method signature from the interface - with is the correct solution in my eyes!). I don't know in what order PHP checks the signature missbehaviour, but it raises the Fatal Error, when reaching the AbstractNavigationFactory

alextech commented 6 years ago

createService() was left there for legacy code used with Service Manager v2 to avoid BC crashes on new releases of Zend Navigation if you cannot also upgrade to SM v3. Hence the comment on lines 49 and 36 stating the version number on the end. If you are using ServiceManager v3, which seems to be your case, then you need to use __invoke method, as current version of SM wants to have service factories as invokable classes.

EDIT: Ok, I am seeing that v2 of service manager also had single parameter. Sorry, I thought v2 had more than one parameter. I will have to see if creating Navigation Service without those optional names will break anything.

froschdesign commented 6 years ago

@alextech We should remove / replace the old interface instead of support legacy versions of service-manager.

alextech commented 6 years ago

@froschdesign for next version? For current version I think it was legitimate refactoring error, where extra parameters were left in, and navigation service was created with dynamic name, despite knowing we are creating THIS navigation service.

I think the PR addresses that, or do you want to remove the v2 signature all the way? Sorry, didn't understand your intent.

I also haven't figured out yet how to get PR to pass with both SMs at once. Will try again on my own travis, but tf we drop SMv2, then trying to figure it out and this bug becomes moot.

froschdesign commented 6 years ago

…remove the v2 signature all the way?

Remove!

alextech commented 6 years ago

Ok, That would require increasing dependencies (maybe with PHPUnit et. al). I can try do that the way I saw other repositories have. I guess that would make it for release 2.9.0? I can help with that, sure.

Shouldn't this still be fixed for those who are still stuck on SMv2 with PHP 7.2? If you say no, then this can be closed as "wont fix".

DennisDobslaf commented 6 years ago

What's the fallback solution for those still using SMv2 if you'll remove the signature? I only speak for myself, but we have some huge projects. Guess it's recommended to switch to v3?

froschdesign commented 6 years ago

What's the fallback solution…

Guess it's recommended to switch to v3?

Right. (version 3 of zend-servicemanager was released on 11 Jan 2016)

Please don't get me wrong, I am a fan of a backwards-compatible manner and semantic versioning. So the first step is to mark the old interface / methods as deprecated – version 2.9.0. Version 3.0.0 will drop the old service-manager support.