Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change Controller::__construct() to accept an array as its argument #7312

Closed
wants to merge 1 commit into from

Conversation

rchavik
Copy link
Member

@rchavik rchavik commented Aug 27, 2015

@markstory markstory added this to the 3.0.13 milestone Aug 27, 2015
*/
public function __construct(Request $request = null, Response $response = null, $name = null, $eventManager = null, $components = null)
public function __construct($options = [])
Copy link
Member

Choose a reason for hiding this comment

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

This adds a bunch of code and makes type-hinting harder, what is the benefit?

Copy link
Member Author

Choose a reason for hiding this comment

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

I can't exactly remember where I say see this: Future Proof APIs, Options parameters make growing easy.

:)

@robertpustulka
Copy link
Member

I'm 👎 on this. It makes harder to work with reflection.

@ADmad
Copy link
Member

ADmad commented Aug 27, 2015

Given that the developer almost never needs to instantiate controller "by hand" I too don't see the benefit of accepting args as array.

@lorenzo
Copy link
Member

lorenzo commented Aug 27, 2015

On the fence here, I think that if we had a compelling reason to do this in the future we could just do it. So far I don't see a big benefit, on the other hand it will break my dependency injection plugin as no typehints will be found :(

@markstory
Copy link
Member

The bag of stuff parameter works ok when we have a servicelocator/factory like TableRegistry. But we don't have one of those for controllers.

@rchavik
Copy link
Member Author

rchavik commented Aug 28, 2015

It feels ugly. Especially the name argument, that looks really out of place there. A special arg for testing? Feels like 'cheating'.

Pity php has no named params. :(

@lorenzo
Copy link
Member

lorenzo commented Aug 28, 2015

Named params would make everything so much better :D

@dereuromark
Copy link
Member

Indeed.
Well, seems like we should hold off on that change and alike until 4.x.

@rchavik
Copy link
Member Author

rchavik commented Aug 28, 2015

Closing :(

@rchavik rchavik closed this Aug 28, 2015
@josegonzalez josegonzalez deleted the 3.1-controller-constructor branch August 28, 2015 15:28
@robertpustulka
Copy link
Member

A class should never have too many dependencies. 3-4 are OK, 5 are max. If you need more then your class is badly designed. I don't see why we should make such future-proof changes. If the need emerges, we'll find another, more elegant way to deal with many dependencies.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants