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

[ZF3] [Debate] Remove ServiceLocatorAwareInterface from AbstractController #5168

Closed
macnibblet opened this issue Sep 26, 2013 · 90 comments
Closed
Labels
Milestone

Comments

@macnibblet
Copy link
Contributor

Heya people

After a rather interesting discussion in #5156 many people tend to agree that having the AbstractController implement the ServiceLocatorAwareInterface is a bad idea since it allows for poor practise.

Yes, it can be convenient at times but it's used far too carelessly. Removing the ServiceLocatorAwareInterface we can "enforce" better practise among our users ?

@blanchonvincent
Copy link
Contributor

Agree with removing the SM in the controller:+1:

@Ocramius
Copy link
Member

@macnibblet fear the rage of those who do RAD!

I think we still need to keep its "dirty little brother" somewhere...

@macnibblet
Copy link
Contributor Author

@Ocramius, so people who do RAD don't write tests ....?

@stefanotorresi
Copy link
Contributor

yea, remove it altogether, otherwise I will spam with some more strange ideas! :p

👍 btw

@localheinz
Copy link
Member

👍

@Ocramius
Copy link
Member

@macnibblet actually, people who do RAD most people don't write tests.

While I fully support this choice (removing ServiceLocatorAwareInterface from wherever possible), this is actually going to be a plus only for the 10% of developers who do testing and apply best practices.

@GeeH
Copy link

GeeH commented Sep 26, 2013

Remove that bad boy, remove it now!

👍

@Slamdunk
Copy link
Contributor

Remove it.

@Ocramius don't worry about that 90%: the next day ZF3 will go stable, someone will create a Module to subclass AbstractController with ServiceLocatorAwareInterface; if you are fast enough, you can be the one, calling that Module DontUseMe\WriteTestsInstead\Module

@Thinkscape
Copy link
Member

I smell nitpicking here. There are bigger, more general issues with using ServiceManager (or service containers in general), ducks vs interfaces and "magic-injection" based on *AwareInterface... there's also topic of building fast DI engine that would solve other group of issues etc.

Picking apart smaller implementation details from zf2 does not amount the the quality and quantity of discussion we have had when architecting zf2. We had tens of threads, RFC in (RIP) wiki and general architecture discussions there.

I don't like the idea of building zf3 by removing bits and pieces of zf2 and calling that progress. If you want to tackle the problem, let's start real discussion :-)

@bakura10
Copy link
Contributor

+1 !

As I said in another discussion, we need to have a better ZFTool that does:

  • php create controller Foo --factoy (or --invokable)

that: create controller, create factory with basic code, add the key in controllers key. This way a lot of creating file burden is removed, and we keep the nice architecutre.

@macnibblet
Copy link
Contributor Author

@Slamdunk 👍
@Thinkscape: Yes i agree with you

@juriansluiman
Copy link
Contributor

I am not sure this goes into the right direction. Yes, "we" here are all good developers, actively contributing to the framework. Understand that the user base of zf2/zf3 is much, much larger than the ppl you know here. The learning curve for zf1 was high, zf2 was considered even more difficult. We shouldn't remove RAD features just to enforce users writing proper code. Eventually, it will just end in less usage of the framework, making the user base smaller, causing all kinds of other problems.

If we consider these topics to enforce proper code, we should actively consider replacing tools to bring back the RAD style of development. You should be able to just point out some dependencies in a config file which are automatically injected. I don't know, but we should not remove the parts that are the easiest to learn by new users. Not everybody is perfect, you know :-)

How I see most programmers use frameworks like zf2: first they create all objects themselves, so all new MyForm; everywhere. Then they start learning the SL and use that to grab the form. Eventually they learn DI and inject all dependencies into the constructor. You seriously cannot teach programmers this third stage if they are not allowed to fail (and learn from that) at stage one or two.

@zluiten
Copy link
Contributor

zluiten commented Sep 26, 2013

@juriansluiman 👍 Couldn't have said it better :)

@pauloelr
Copy link
Contributor

My "Not Especialist Developer" Humild Opnion:

I'm really not design pattern especilist nor consider myself a deep expert of programing, in fact i'm just a developer who tries to increase quality of the code produced along the time, many of the evolution of my code is due the shared knowlodge of some experts like you that are right now discussing things i don't fully understand, but i try, i try the same way i try to contribute with ZF2 code in minor bug fixes or even on documentation upgrades, and i did it, to help of course, but to learn either.

As a "not expert" developer i like when the things are easy untill someone more expert then me tell me that there is a better practice, and explain me why i shoudn't do the way i do.

What i'm trying to say is that many of developers like me use the ServiceLocatorAwareInterface thinking that this is a best practice since a huge framework like ZF2 implements it by default, i think you're doing a really great job keeping the framework easy to use and reducing the learning curve, but i also think you, as experts, need to think greater than that and realize that what you do, the way you implement things is being more than a base code to develepers build their application but also a study place for many ones that like me wanna grow as a developer.

This way i think this discussion is far more bigger then remove or not remove the ServiceLocatorAwareInterface form AbstractController, is a discussion about how you want to be the future of PHP Programing, lets assume you decide to keep the things the way it is, i think you should consider explain in docs why it is the way it is and the other (better) ways to do the things.

The same is valid for the other side, if you decide to remove the ServiceLocatorAwareInterface from AbstractController, if you remove it, and explain why you diid this, and how is the correct way to do things and why, i'm pretty sure the "90% of developers who doesn't make tests" will understand the change and start to create better codes.

But again this is just My "Not Especialist Developer" Humild Opnion, i just felt the need to express the position of medium developers to contrast with the experts opinions, i'm holping it helps.

@bakura10
Copy link
Contributor

@pauloelr 👍 Couldn't have said it better :)

@localheinz
Copy link
Member

"100% of developers think they are in the top 10% of developers."

@unixmen
Copy link

unixmen commented Sep 26, 2013

i think alternatives will not be complicated :

  1. Replacing ServiceLocatorAwareInterface by ServiceLocatorAwareTrait, 2) not using ServiceLocatorAwareTrait in AbstractController and 3) let people just have to use the adequate trait in their controller (and so the corresponding intializer will work in individual controllers scope )

learning namespaces, SL, EventManager, etc was an important step to learn zf2
using traits for this kind of needs, In my opinion, is accessible for all developpers

as ZF3 is gonna use php5.4 or higher , we should discover/develop better oop practices

@Ocramius
Copy link
Member

@unixmen I really love this suggestion! That really changes my PoV on this :O

@bakura10
Copy link
Contributor

Interesting suggestion unixmen, but for the initializer to work we still need interfaces. Traits can't implement interfaces, sadly.

@unixmen
Copy link

unixmen commented Sep 26, 2013

@Ocramius @bakura10 get used traits used in controller with class_uses() and test existing ServiceLocatorAwareTrait key to inject SL in the controller

http://php.net/manual/en/function.class-uses.php

@unixmen
Copy link

unixmen commented Sep 26, 2013

i understand the problem, the initializers process will need to be refactored

(i don't master yet PHP5.4) , i imagine a kind of initializer doing injections in a simple way like this :

if ( array_key_exists('ServiceLocatorAwareTrait' , class_uses($controllerName) )
// or class_uses($controllerName)['ServiceLocatorAwareTrait'] ...
{
$controllerName->setServiceLocator($sl)
}

@texdc
Copy link
Contributor

texdc commented Sep 26, 2013

I support the move to traits, but not at the expense of interfaces. They add clarity to code. Be explicit!

@unixmen
Copy link

unixmen commented Sep 27, 2013

i realize it can't be easily done like hoped,

i know there are lazy (and bad) ways to use SL via traits but i'm allergic to provide solutions like to use static ServiceLocator ( also static SL with traits is doesn't make sense )

what do you think to remove the ServiceLocatorAwareInterface from AbstractController and as most users use AbstractActionController, using the ServiceLocatorAwareInterface there as a step will improve implementing own's controllers for all reasons we know

@Thinkscape
Copy link
Member

@texdc is right.

Sorry to be the one bursting your bubble.

After working with traits (in php) for some time now, I've learned that traits are quite useless for api's - they are "invisible" to the parser, you cannot hint on them, they do not support inheritance, they have fixed subclass/trait calling-tree , they are sketchy when used together with interfaces.

The only way to build framework code consuming interfaces is to use in_array('...', class_uses()) or reflection, which is basically duck typing and does not differ from is_callable([$this, 'setServiceLocator']) ... I fear that jumping on traits will result in us becoming DuckFramework 3 :P

@macnibblet
Copy link
Contributor Author

Why don't we simply move the it to a plugin ? it would actually coherce a lot better with the rest of the controller code

@glen-84
Copy link
Contributor

glen-84 commented Sep 27, 2013

Not until you rewrite ZF-Commons/ZfcUser (for example), to show how you're supposed to work with controllers that have more than 1 or 2 dependencies (and dependencies that may or may not be used).

@froschdesign
Copy link
Member

@glen-84
I'm on your side, but the reference guide is the correct and the official place for this.

@GeeH
Copy link

GeeH commented Sep 27, 2013

I don't understand why there's discussion. It's an anti-pattern and should be removed, leaving it in is tantamount to endorsement of the pattern. The Service Locator will still support coding "aware" interfaces, so it can be added back in if it means that much to an individual shitty developer.

@macnibblet
Copy link
Contributor Author

@Spabby, didn't you just learn to write test recently ? :trollface:

@GeeH
Copy link

GeeH commented Sep 27, 2013

💥

Testing is irrelevant, the pattern is a poor one and to endorse it is wrong.

@samsonasik
Copy link
Contributor

👍 as far as there are guaranteed resources for beginners / new users.

@texdc
Copy link
Contributor

texdc commented Oct 4, 2013

👍 SoC

@pauloelr
Copy link
Contributor

pauloelr commented Oct 4, 2013

👍

@glen-84
Copy link
Contributor

glen-84 commented Oct 4, 2013

👍 ... if the skeleton application will be updated at the same time.

@unixmen
Copy link

unixmen commented Oct 5, 2013

👍

maybe a base skeleton + a modules for that purpose (or some config that can be disabled )

(using many skeleton apps could be another possible slackness )

@EvanDotPro
Copy link
Member

👍 better late than never.

@amazium
Copy link

amazium commented Oct 17, 2013

👍

@danizord
Copy link
Contributor

I'm 👍 with remove the ServiceLocatorAwareInterface, trait, initializer and all about it.

@macnibblet I like your idea of ​​a controller plugin. This will keep the ease and quickness for who want it, but without sacrificing the performance of those who want to do it the right way.

public function fooAction() {
    $service = $this->services()->get('MyService');
}

Thoughts?

@markdrake
Copy link

👍

@rodsouto
Copy link

@danizord the anti-pattern is still present, a controller shouldn't have access to the SM in any way :)

👍

@HardieBoeve
Copy link

👍

@amazium
Copy link

amazium commented Oct 17, 2013

@danizord the idea is that you don't use it in the controller at all. Not injected, but not as plugin either. You should inject classes you pull out of the SM into the controller, not let the controller do the pulling.

@danizord
Copy link
Contributor

@rodsouto and @amazium Yes, I know the best practice! I'm just suggesting to create only a plugin istead of:

  • Create a ServiceAwareTrait, but don't use it in AbstractController
  • Create an initializer for the trait but don't register it.
  • Create another SkeletonApp for people who want to use the anti-pattern.

@stefanotorresi
Copy link
Contributor

@danizord
this has already been established since this comment

@danizord
Copy link
Contributor

@stefanotorresi 😱 I'm suggesting quite the opposite. I suggested creating only the plugin instead of it all.

@stefanotorresi
Copy link
Contributor

@danizord yes, I get it, but a plugin would probably be 'too easy' and wouldn't be enough of a clear message against the whole practice. The point of what has been discussed so far is that the current pattern should be kept feasible while being blatantly discouraged. This way, at some point in the future it could be even get ridden of completely.

@glen-84
Copy link
Contributor

glen-84 commented Oct 20, 2013

I've posted a question about dependency management on Stack Overflow here. It would be great to get some insight from the experienced ZF developers in this thread WRT how you handle dependencies in your own applications.

@bacinsky
Copy link
Contributor

bacinsky commented Nov 4, 2013

I think this is not a good idea. I use a serviceLocator for RAD prototyping, and when it's appropriate I redesign to injections. This saves me a lot of trial and error time than playing with configs etc. The whole PHP is about to be able to write a quick dirty code. Instead of forcing devs to write time consuming clean testable code, write a better documentation to explain some bad practice when is bad, because it's not as bad always. There should be always some space for a choice.

@DASPRiD
Copy link
Member

DASPRiD commented Nov 5, 2013

@bacinsky The choice is always there, just use the interface and the trait.

@bacinsky
Copy link
Contributor

bacinsky commented Nov 5, 2013

@DASPRiD I'm fine with that. That's clean.
👍

@agustincl
Copy link

cool

@localheinz
Copy link
Member

@macnibblet

I think you should fix the title of this issue:

  • [ ZF3 ] vs [ZF3]
  • [ Debate] vs [Debate]

@macnibblet macnibblet changed the title [ ZF3 ] [ Debate] Remove ServiceLocatorAwareInterface from AbstractController [ZF3] [Debate] Remove ServiceLocatorAwareInterface from AbstractController Jul 13, 2014
@RalfEggert
Copy link
Contributor

@macnibblet

And maybe the milestone could be set to 3.0.0 as well? makes it easier to find the issue.

@Ocramius Ocramius added this to the 3.0.0 milestone Jul 14, 2014
@Ocramius
Copy link
Member

done

@Maks3w
Copy link
Member

Maks3w commented Oct 9, 2015

Those interfaces not longer exists on SM v3

@Maks3w Maks3w closed this as completed Oct 9, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests