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

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

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

Comments

Projects
None yet
@macnibblet
Copy link
Contributor

commented Sep 26, 2013

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

This comment has been minimized.

Copy link
Contributor

commented Sep 26, 2013

Agree with removing the SM in the controller👍

@Ocramius

This comment has been minimized.

Copy link
Member

commented Sep 26, 2013

@macnibblet fear the rage of those who do RAD!

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

@macnibblet

This comment has been minimized.

Copy link
Contributor Author

commented Sep 26, 2013

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

@stefanotorresi

This comment has been minimized.

Copy link
Contributor

commented Sep 26, 2013

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

👍 btw

@localheinz

This comment has been minimized.

Copy link
Member

commented Sep 26, 2013

👍

@Ocramius

This comment has been minimized.

Copy link
Member

commented Sep 26, 2013

@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

This comment has been minimized.

Copy link

commented Sep 26, 2013

Remove that bad boy, remove it now!

👍

@Slamdunk

This comment has been minimized.

Copy link
Contributor

commented Sep 26, 2013

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

This comment has been minimized.

Copy link
Member

commented Sep 26, 2013

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

This comment has been minimized.

Copy link
Contributor

commented Sep 26, 2013

+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

This comment has been minimized.

Copy link
Contributor Author

commented Sep 26, 2013

@Slamdunk 👍
@Thinkscape: Yes i agree with you

@juriansluiman

This comment has been minimized.

Copy link
Contributor

commented Sep 26, 2013

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.

@netiul

This comment has been minimized.

Copy link
Contributor

commented Sep 26, 2013

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

@pauloelr

This comment has been minimized.

Copy link
Contributor

commented Sep 26, 2013

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

This comment has been minimized.

Copy link
Contributor

commented Sep 26, 2013

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

@localheinz

This comment has been minimized.

Copy link
Member

commented Sep 26, 2013

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

@unixmen

This comment has been minimized.

Copy link

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

This comment has been minimized.

Copy link
Member

commented Sep 26, 2013

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

@bakura10

This comment has been minimized.

Copy link
Contributor

commented Sep 26, 2013

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

@unixmen

This comment has been minimized.

Copy link

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

This comment has been minimized.

Copy link

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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link

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

This comment has been minimized.

Copy link
Member

commented Sep 27, 2013

@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

This comment has been minimized.

Copy link
Contributor Author

commented Sep 27, 2013

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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Member

commented Sep 27, 2013

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

@GeeH

This comment has been minimized.

Copy link

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

This comment has been minimized.

Copy link
Contributor Author

commented Sep 27, 2013

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

@GeeH

This comment has been minimized.

Copy link

commented Sep 27, 2013

💥

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

@samsonasik

This comment has been minimized.

Copy link
Contributor

commented Oct 4, 2013

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

@texdc

This comment has been minimized.

Copy link
Contributor

commented Oct 4, 2013

👍 SoC

@pauloelr

This comment has been minimized.

Copy link
Contributor

commented Oct 4, 2013

👍

@glen-84

This comment has been minimized.

Copy link
Contributor

commented Oct 4, 2013

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

@unixmen

This comment has been minimized.

Copy link

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

This comment has been minimized.

Copy link
Member

commented Oct 17, 2013

👍 better late than never.

@amazium

This comment has been minimized.

Copy link

commented Oct 17, 2013

👍

@danizord

This comment has been minimized.

Copy link
Contributor

commented Oct 17, 2013

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

This comment has been minimized.

Copy link

commented Oct 17, 2013

👍

@rodsouto

This comment has been minimized.

Copy link

commented Oct 17, 2013

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

👍

@HardieBoeve

This comment has been minimized.

Copy link

commented Oct 17, 2013

👍

@amazium

This comment has been minimized.

Copy link

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

This comment has been minimized.

Copy link
Contributor

commented Oct 17, 2013

@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

This comment has been minimized.

Copy link
Contributor

commented Oct 17, 2013

@danizord
this has already been established since this comment

@danizord

This comment has been minimized.

Copy link
Contributor

commented Oct 17, 2013

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

@stefanotorresi

This comment has been minimized.

Copy link
Contributor

commented Oct 17, 2013

@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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Member

commented Nov 5, 2013

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

@bacinsky

This comment has been minimized.

Copy link
Contributor

commented Nov 5, 2013

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

@agustincl

This comment has been minimized.

Copy link

commented Nov 25, 2013

cool

@localheinz

This comment has been minimized.

Copy link
Member

commented Jul 13, 2014

@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

This comment has been minimized.

Copy link
Contributor

commented Jul 14, 2014

@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

This comment has been minimized.

Copy link
Member

commented Jul 14, 2014

done

@Maks3w

This comment has been minimized.

Copy link
Member

commented Oct 9, 2015

Those interfaces not longer exists on SM v3

@Maks3w Maks3w closed this Oct 9, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.