Skip to content
This repository was archived by the owner on Jan 29, 2020. It is now read-only.

quick hack for psr-4 autoloading #252

Closed
wants to merge 5 commits into from
Closed

quick hack for psr-4 autoloading #252

wants to merge 5 commits into from

Conversation

chris-kruining
Copy link

I have a small issue with having to register controllers in my config while the are perfectly auto loadable via psr-4. some extensive googling didn't yield any results so I wrote a small hack. Of which I have to say: No idea what the performance impact is or if the testcase even remotely works.

So to finalize, is this a valid hack/implementation of dispatching to psr-4 autoloadable classes? if so do you guys want to implement it, or did I totally miss an option/setting/config that allows for autoloading(without having to config it as invokable)?

@chris-kruining
Copy link
Author

ooooh, sorry for making a pull request straight on the development branche :S

@Ocramius
Copy link
Member

Ocramius commented Aug 7, 2017

You can push up another branch, then edit the PR

@chris-kruining
Copy link
Author

nevermind, I thought PR's had to be made to a specific branch, but that is develop, so no problems :D

@chris-kruining
Copy link
Author

@Ocramius I can't seem to get the testcase to work, I 'm probably just missing something trivial. But what do you think of the idea to allow skipping config for classes that are autoloadable?

@adamlundrigan
Copy link
Contributor

@chris-kruining your test asserts that the result is empty when it is not:

There was 1 failure:

1) ZendTest\Mvc\DispatchListenerTest::testControllerManagerUsingPsr4LoadedClass
array (
  'error' => 'error-controller-not-found',
)
Failed asserting that an array is empty.

/home/travis/build/zendframework/zend-mvc/test/DispatchListenerTest.php:83

Also, code coverage decreased

@@ -86,6 +86,12 @@ public function onDispatch(MvcEvent $e)
$events = $application->getEventManager();
$controllerManager = $this->controllerManager;

if (class_exists($controllerName) && ! $controllerManager->has($controllerName))
{
Copy link
Contributor

Choose a reason for hiding this comment

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

@Xerkus
Copy link
Member

Xerkus commented Sep 7, 2017

👎
Controller manager is intentionally implemented without autoinvokable feature. Trying to sneak it in here not only out of place, it is outright dangerous.
Same reasoning goes here as to why controller manager was split out from main service manager in the first place.

@thomasvargiu
Copy link

👎
I agree with @Xerkus, controller classes are dependencies of the controller manager, so they should be registered in the container.

@froschdesign
Copy link
Member

Controller manager is intentionally implemented without autoinvokable feature.

Right!

class ControllerManager extends AbstractPluginManager
{
/**
* We do not want arbitrary classes instantiated as controllers.
*
* @var bool
*/
protected $autoAddInvokableClass = false;

@chris-kruining
Copy link
Author

@Xerkus @thomasvargiu @froschdesign okay I can not and will not argue the validity of your arguments, but I think you can agree with me that having to register each and every single controller(in my case a lot, near 50-70 I believe) It is a giant hassle having to do all that extra typing when adding a new controller or refactoring stuff. for example I added a new module which needed a controller or 10 that I added sequentially over a period of a week or two, and All I do is add a line of code FQCN::class => \Zend\ServiceManager\Factory\InvokableFactory::class. So can we think of a way to create a way to validate a class to be a controller without having to register each and every single class in the config? I can imagine some things, like detecting if the class trying to be invoked as a controller implements an interface somewhere in its inheritance chain. Or maybe add some config where I can define a pattern that will be able to match a path where controller can be found. I hope we can figure out a method together :D

@froschdesign
Copy link
Member

froschdesign commented Sep 8, 2017

@chris-kruining

It is a giant hassle having to do all that extra typing when adding a new controller or refactoring stuff.

If your problem is typing, then why would you change the ControllerManager? 😉

In the past we had a tool for this: ZFTool
(Here a PR for your use case: zendframework/ZFTool#80)

The same for expressive: zend-expressive-tooling

@Ocramius
Copy link
Member

Ocramius commented Sep 8, 2017 via email

@Xerkus
Copy link
Member

Xerkus commented Sep 8, 2017

@chris-kruining service locator is not allowed into my controllers, so I not only add config entries, i am actually creating factories. Service locator is a problem for testability, especially in the long run.

During development when you have to create, remove and rename controllers quickly, reflection based LazyControllerAbstractFactory is what you need. Just replace it with actual factory when feature stabilizes enough to be merged.
Or, as was mentioned, abstract factory is a way to go.

I am going to close this pull request now.

@Xerkus Xerkus closed this Sep 8, 2017
@chris-kruining chris-kruining deleted the develop branch September 13, 2017 09:19
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants