-
Notifications
You must be signed in to change notification settings - Fork 10
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
A method to enforce factory methods signature? #18
Comments
Hey Giuseppe! Thanks a lot for taking the time to put up this proposition. Here, I'm not going to address the "static" vs "non-static" part (since it is already addressed in #5. If I understand correctly, you are trying to add "type safety" to the service provider by making the Then, the
The nice thing about your proposal is that it feels a bit less hackish than returning a string of However, it is also slightly slower (in the case of a compiled container, the container will need to call the createService method that will itself call the factory method instead of calling the factory method directly). @mnapoli any thoughts about this Regarding your argument about the fragility of the current proposal, I must say I'm not convinced at all. First, you are adding additional code to perform type checking which will slow down the container (a lot of people are very picky when it comes to container performance). Then, this is something that should be really dealt by PHP directly. How does it compare to our current proposal in PHP 7? class MyServiceProvider implements ServiceProvider
{
public static function getServices()
{
return [
'my_service' => 'getMyService',
];
}
public static function getMyService() : MyService
{
return new MyService();
}
} Using this implementation:
|
My implementation was just an example. See this one: class MyServiceProvider implements ServiceProvider
{
public function servicesMap()
{
return ['logger' => LoggerInterface::class];
}
public function createService($service, ContainerInterface $c, callable $previous = null)
{
if ($service === 'logger')
return new Logger('name');
}
} I think that there's no performance concern here. Finally, in my experience, talking about performance without any real world profiling makes no sense: excluding "stupid" code, most of the times performance bottlenecks are where you don't expect them and assumption are not something that worth talking about. And, surely, if we had only PHP 7+ as target, my My point is not only type sanity, but the fact that current implementation is just a "fake" interface. A "real" interface is a behavioural contract that object instances have to respect. The current implementation (besides not considering any object instance because of statics) does not contain any "contract" in code: Still it isn't the perfect design, but at least I find it less hackish and more intention revealing. Anyone looking your example immediately undersand what that service provider does... but that is an implementation: if you have to look at implementation to understand what an object does, than the abstraction is wrong, because the most important thing that abstraction should do is to make clear what implementations are expected to do. Please imagine a developer (even experienced) who never heard about container interop and see this code. They could use all the experience and all the imagination they has, but they will never ever could be capable to guess what you expect And even the naming does not help... |
Could you be more constructive please? This is just wrong information, and derailing the topic. |
Actually, I was referring to Symfony. They are very much counting the number of function calls performed when it comes to compiling their container and trying to minimize the impact of the container on global performance as much as possible. |
@mnapoli removed that sentence. And I don't think performance is the main and not even a minor topic here. Performance will entirely depends on implementations, and we are talking about interfaces. |
Hey, Giuseppe, if you look at your proposed implementation, it is actually not that far from an "extended" ContainerInterface: // The interface extends ContainerInterface (a service provider can be called with get/has)
// The interface extends Traversable: when traversed, the values yielded are the name of the services
interface ServiceProviderInterface implements ContainerInterface, Traversable
{
// The get method takes an additional parameter: the previous declaration of a service.
public function get($identifier, $previous = null);
} This is not really a "service provider" anymore, but it fills the same purpose. What I would however is maybe to put a pause on this discussion for now. There are a lot of things to be discussed regarding the final look of the So could we postpone any discussion regarding the |
David, please, don't look at implementation. Current interface is: interface ServiceProvider {
public static function getServices();
} My proposal is: interface ServiceProvider {
public static function servicesMap();
public static function createService($service, ContainerInterface $c, callable $previous = null);
} I used statics methods in my proposal here as well for a more direct comparison. Note how the current implementation already expects that service provider classes contain factory methods that have the exact same signature of It means that the class design (in term of class responsibility) is identical. The difference is that my implementation makes things explicit, while current implementation does not. So people looking at the current interface will need to read documentation or some meta document on the web to understand how to implement factory methods, looking at my interface they will just need to look at code. This "explicitness" I think is the main benefit of my proposal and capability to enforce the signature of factory methods is a, non trivial, additional benefit. In summary, with the interface I propose you loose nothing and you just gain something. |
What you loose is that all the factory code is crammed into one method though (if you want to avoid sub-method calls). |
Well, assuming a service provider would provide just sever providers, the code could be: public static function createService($service, ContainerInterface $c, callable $previous) {
switch($service) {
case 'a':
return new A();
case 'b':
return new B();
case 'c':
return new C();
}
} or maybe: public static function createService($service, ContainerInterface $c, callable $previous) {
if (method_exists(__CLASS__, $service)) {
return call_user_func([__CLASS__, $service], $c, $previous);
}
} Implementation is left to implementers. Just to say some numbers: in current implementation the number of factory methods is always, at least Using the implementation I proposed, number of methods could be minimum My point is:
|
I don't understand your numbers.
Yes your suggestion has the benefit of being able to enforce the signature of the factory method but having to put all the service factories inside one method is a big flaw IMO. Not being able to enforce the signature of the factory isn't such a big deal, that's how it works today with Pimple (Silex, Slim…), Laravel, PHP-DI, etc. Zend ServiceManager also allows PHP callbacks (so no interface in that case). It doesn't stop those projects from working. |
Just to explain the numbers. First of all there's no "my 1st solution" and "my 2nd" solution: those "1st" and "2nd" are implementation examples. You can't stop people on do specific things (or crazy things) on implementantion, so you should not care about implementation at all. There's only one proposal of mine, and it is the interface. Using my proposal, that regards the interface, people are free to have all factory methods inside the Let's admit that put all the service factories inside one method is a big flaw: this is a implementation detail, not something that is enforced by the interface, you can't blame the interface for an implemementation choice on which the interface has no power. Are you really convinced that current interface is good enough? Fine. I'm not. Really not. I tried to convince you, without success. I think that until me, you and @moufmouf are the only three people involved in the discussion, there's no much more to say. The point is that the real number of methods depends on implementation. And since you can't know how people will implement the interface, makes absolutely no sense to talk about the implementation details such as the number of methods. Considering that the final target of the standard of is an interface, we should only talk about which is the best interface not the best implementation. (Update 12/04/2016 everything below here is something stupid I wrote without thinking too much) Hoping that my point is clear, let's go back to numbers. Assuming that a service provider provides Current implementation needs:
In total: A service provider that implements the interface I proposed, to be fully resolved, may require:
So in best scenario the interface I proposed uses just 2 method calls vs 4 of the current implementation. Which may became 2 VS 11 for a service provider that provides 10 services. In worst scenario, my implementation requires 5 methods (for 3 services) VS 4 of current implementation (assuming best scenario for current implementation). Which became 12 VS 11 for a service provider that provides 10 services. In short, in worst scenario requires just a method more, but in best scenario can save much more method calls. |
We need to consider how interfaces will be implemented. It doesn't make sense to consider the interface alone ore we'll only be talking about smoke. If it's not possible to write an implementation that is clear and efficient then the interface needs to be improved.
Exactly:
On the other hand the current solution doesn't have this problem because it can enforce it.
We don't need to consider this call because it will most probably not be done at runtime… (for obvious reasons)
I don't understand.
Maybe with your solution we can't know but with the current one we know.
This is wrong on many levels. Caring about a standards but not about implementations is the best way to fail. We have to take real implementation constraints into account. Have you tried implementing your suggestion into your DI container or your framework (or any existing container or framework)? The PHP-FIG members don't want to waste time on theoretical ideas, that's why we spend time actually implementing and testing what we design. We spend considerable amount of time writing adapters and implementations to have real experience on what we suggest. Drafting something on a paper on a GitHub issue is 5% of the job. That's why we are discussing details. |
Hey guys, I understand very well both points of view. On @Giuseppe-Mazzapica 's side, it's very clear this is all about having an interface that is "readable" and "explicit". Looking at the method signature of the interface proposed by Giuseppe, it is immediately obvious what the interface does. Looking at the current proposed interface, the method signature alone is not enough. You have to read the PHP-Doc to understand how it works. Now, I've worked a lot on different implementations (both with runtime and with compiled containers). If you "compile" the container, it means that you want your container to have literally 0 impact on performance. Using Giuseppe's proposal, we would certainly have something like this most of the time: public static function createService($service, ContainerInterface $c, callable $previous) {
return call_user_func([__CLASS__, $service], $c, $previous);
} Now, for each service, we get:
Let's compare this to the current interface proposal. For runtime containers, the call to For compiled containers however, the call to Always regarding performance, with the current interface proposal, we can add some performance improvements for compiled container. I've been playing lately with a service provider to definition-interop container. This is completely alpha and a bit crazy, but it works. Basically, I'm reading the service provider and trying to understand the "meaning" of the code. function bar(Container $container) {
return $container->get('foo');
} then I can tell that the 'bar' factory is an alias to 'foo'. This is interesting because compiled container can resolve aliases at compile time rather than at runtime, hence saving some more function calls. You get the idea. The current interface offers a great deal of optimization strategies. If we add a So that's it basically. We are facing a dilemma. We must make trade-off between expressiveness and performance. There is no "right" choice, just a trade-off. Very honestly, I like both solutions, and if any one of those solutions makes it as a PSR, I'll be happy. |
Hi guys, I was at Symfony Live in Paris last Friday, and I had a chance to speak with @stof about service-providers vs definition-interop. I was explaining what I'm trying to do right now - static analysis of PHP code to "guess" the definitions from the factory code (which is a bit hackish), when he came up with a great idea (IMHO) -. Here goes his idea: Rather than having a class MyServiceProvider implements ServiceProvider
{
public (maybe static) function getServices() {
return [
"my_alias" => function(ContainerInterface $c, callable $previous = null) {
return $c->get('foo');
}
]
}
} or class MyServiceProvider implements ServiceProvider
public (maybe static) function getServices() {
return [
"my_alias" => [self::class, 'alias']
]
}
public static function alias(ContainerInterface $c, callable $previous = null) {
return $c->get('foo');
}
} Why is it a good idea? Because it allows the creation of invokable classes. Like this: interface ServiceProviderFactory {
public function __invoke(ContainerInterface $container, callable $previous = null);
}
class Alias implements ServiceProviderFactory, ReferenceDefinitionInterface /* from definition-interop! */ {
private $target;
public function __construct($target) {
$this->target = $target;
}
public function getTarget() {
return $this->target;
}
public function __invoke(ContainerInterface $container, callable $previous = null) {
return $container->get($this->target);
}
} Now, the service provider can look like this: class MyServiceProvider implements ServiceProvider
public (maybe static) function getServices() {
return [
"my_alias" => new Alias('alias');
]
}
} This is very interesting because this enables us to build a "bridge" between service providers and definition-interop. Objects can implement both the __invoke method and (optionnally) some interface from definition-interop. Runtime containers will simply ignore the definition-interop part. Compiled container can tap into it to make additional optimizations if needed (resolving aliases or parameters, maybe doing some inlining, etc...) So on the positive side, we have:
On the downside:
So that's it. I'm pretty fond of this @stof idea. I think it is way cleaner than what I'm doing with static code analysis (and way more robust!) I'd definitely like to give it a go. What do you think? |
Hi @moufmouf maybe we should start another thread about your suggestion? Anyway this was a very interesting read and very interesting idea with a lot of potential. However I'm a bit cautious because in order for your solution to work everyone (i.e. every module) needs to be using the objects instead of the callables. So writing modules has gone back to being complicated like in definition-interop, we loose a lot of the simplicity of service-provider. Yes modules are not forced to use objects, because we only require them to be callables, however if some containers require definitions to be objects then we have 2 worlds and the standard is not a standard anymore. And while your example works, it's simple. Aliasing an entry is quite rare so not really a performance issue. I believe the biggest performance issue might be with decorating an object repeatedly (e.g. event dispatcher to which each module registers listeners) or decorating an array repeatedly (e.g. "twig.extension" array to which modules add items). Both of those are not as simple to represent using object definitions. A solution I was thinking about but I'm not sure if it could ever be accepted could be:
But obviously annotations… Anyway the idea behind that would be to have 2 standards: one basic that works everywhere, and another one that comes on top of the first that adds more specific behaviors. I don't think we should make too many sacrifices because of compiled containers (mainly Symfony after all). I'm fine with finding a middle ground, but as @mindplay-dk said in container-interop/definition-interop#26 we shouldn't penalize everyone because of one container. ircmaxell summarizes that very well in http://blog.ircmaxell.com/2014/10/an-open-letter-to-php-fig.html (which was partly about how PSR-6 could have been much simpler yet still powerful). |
Hey Matthieu, I completely agree that we should split this into 2 standards. And actually, right now, we really need to focus on only one standard: service-providers. Now, I've been playing with the return type of "getServices" this afternoon. I've been switching the return type to an array of callables.
We get some direct benefits from this:
One of this freedom is about having the possibility to use objects implementing the You are talking about decorating an array repeatedly ( public static function getServices() {
return [
'twig.extensions' => 'addExtension',
'myExtension' => 'myExtension',
];
}
public static function addExtension() {
return new MyExtension();
}
public static function addExtension(Container $c, $previous = null) {
if ($previous = null) {
$extensions = [];
} else {
$extensions = $previous();
}
$extensions[] = $c->get('myExtension');
} Tomorrow, it could look like this: public static function getServices() {
return [
'twig.extensions' => new AddServiceToArray('myExtension'),
'myExtension' => function() {
return new MyExtension();
}
];
} And maybe So even take the definition-interop argument out of the equation, I still think that returning callables instead of function names is a better idea. So my proposal would be to switch to "array of callables" instead of "array of method names" for the return type of "getServices" and do not touch anything in the PSR. Especially, I'm ok to not add any reference to definition-interop and keep it dead simple. Would you agree with that? |
Replying to @mnapoli
You are perfectly right. I even updated my original comment to say that part is completely BS.
(talking about Can you say which these "obvious reasons" are? I, honestly, don't get it. Me and a few of other millions of people use In your "simplex", Or I am missing something?
(talking about a single method used to factory all services) If it is a design flaw, it is a very minor one. Most of real service providers will have very few services. A method that factory more than one service is not ideal, agree, but assuming it is used to create 3, 4 or even 5 servises, it will stay efficient, readable and maintenable. Again: surely interface I proposed is not perfect, far from it, but I think current implementation is worse on many levels. I'm advocating what imo is the lesser evil.
(talking about number of methods called) Sorry? How do you know how many method will be called. This make no sense. class SomeProvider implements ServiceProvider extends SomeBaseProvider {
public static function getServices() {
return ['aService'];
}
public static function aService() {
$factory = ServiceProviderFactory::factoryFor(__CLASS__);
$instance = $factory->createInstance();
return $instance->setup()->createrService('a'); // inherited from parent class
}
public function __construct() {
$this->init(); // inherited from parent class
}
private function setup() {
$this->anotherMethodForFun(); // inherited from parent class
return $this;
}
} In above implementation there are 6 method calls to instantiate a service. People do crazy things. Note: not might do, just do. Assuming how people will implement an interface (or the number of method called) is impossible and it also is wrong. An interface is a contract to be implemented polymorphically by objects. And polymorphically means "in different ways". The only way to know how people will implement an interface is that there's only one possible way. And if there's only one possible way, then you don't need an interface at all.
You can think at possible implementations. And you can do experiments, and tests.You have to. The point is that you can't make assumption on the "quality" of an interface based on the "quality" of an implementation. The current implementation is very "open" in the sense it is capable to enforce literally nothing, so it can be implemented in so many ways that it is not even possible to imagine. So you can't say "this interface is good because it can be implemented this way" in the same way you can't say "this interface is very bad because it can be implemented that way". You can be sure that shitty implementations will come whichever interface you'll ship in the wild. But as long as you have proven and tested that good implementation may exists this is not a concern. What is very wrong imo, is to make interface decisions just based on the "possible" implementation that will not part of the standard, but left to people who can and will implement in the way they like.
Replying to @moufmouf
Sorry but is the opposite :) Using static methods the only way to call method is With implementation I proposed:
In both cases, no
This is a very good idea IMO. Much better than current implementation. That was also my first idea when I looked at this repo. Then I saw that it was explicitly refused as idea in FAQ so I don't even try to propose it... If I have to be honest, a (dynamic) method that returns an array of callables would probably the best implementation I can think of. Expecially if that method is named |
You don't have to use // Or this way (not tested, but should work too): This should work from PHP 5.3. Just a note... and now following the discussion again. :) |
@ziege Yes, you right, I simplified there, because just few minutes before writing this comment I wrote this other one and there I wrote that actually it is possible to use save callback in a variable for static methods (which, in any case is also possible with dynamic methods and inside an implementation of my interface proposal). |
@Giuseppe-Mazzapica Then I could at least present two additional variants. :) In my current ContainerInterface implementation with ServiceProvider support I use the string variant 'class::method' to efficently save the callables: $services = [ Instead of: This requires about 70% less memory (in PHP 7.0.5). |
@webmozart an opinion? You mentioned you were wary too about the removal of static. |
As promised, I wrote a simple application example to test usage of service providers. You can find it here https://github.com/Giuseppe-Mazzapica/container-interop-experiments There you can find 3 (at the moment) "experiments" each of them contains a Container implementation, a service provider interface and ncessary concrete implementations of it. The repo readme contains more info. |
@Giuseppe-Mazzapica each experiment should be implemented in 2 versions:
Otherwise, your experiments will be biased towards the possibilities of a runtime container |
btw, regarding the proposal we discussed during the SymfonyLive, the great thing about it is that the service provider interface only needs to be specified to return an array of callables. The fact that this callable can be an object with additional methods can be considered as an implementation detail at first. @moufmouf have you started writing code about this experiment ? @Giuseppe-Mazzapica regarding your experiment, the code you wrote makes me think that you misunderstood the concept of the |
Hey @stof!
Yes, I worked most of the day on it. It is more complex than working with static methods, but it's definitely possible. Since a few minutes ago, I have a working prototype on Yaco (a PSR-11 compatible container compiler). I'm planning on migrating the service-provider Symfony bundle next week. You can see examples of such a service provider here (the static keyword needs to be removed from the |
@stof @moufmouf but in order for that solution (callables) to be compatible with Symfony and similar containers that means modules have to use the correct objects (e.g. the |
@mnapoli That's true. But so far, there are very few compiled/cached containers out there (Symfony, Yaco, PHP-DI and Neon I think). So there is a good chance we can converge on something common. Furthermore, even if we do not converge on a de-facto implementation, we are not that far away from a standard with container-interop/definition-interop. So even if we do not have that right away, we have the possibility to add that in a few years. Also, I've proposed a PR for non static |
Might surely be. My understanding is that It means that the factory method can either:
In my experiments I tried to implement all those 4 behaviours. What I get wrong?
I'm not going to write something involving a compiled container. It's like asking a vampire to grow garlic :) But that repo is open to pull requests and I am also open to add whoever ask for it as repository collaborator with write access. |
Quick status update: I've updated the Symfony service-provider bridge bundle to work with non-static callables (provided in #20). I've managed to apply a big deal of optimizations:
So this should ensure that service providers have a very minimal impact on the compiled container. At least, if no service is queried from the service-providers, performance impact is close to 0. |
The proposal looks very different today, and I'm fairly certain these concerns were addressed? For one, we have extensions now. If you feel the current PSR does not address your needs, please open a new thread in Discussions. 🙂 |
I already exposed in #16 my opinion about the fragility I see in current implementation, because the
getServices()
method is not able to enforce... anything.It means that the definition of the standard will entirely be in the method doc block, but the
getServices()
method itself, that is everything the interface contains, is nothing more than a wrapper around an array.I think that if we are expecting people to write factory methods in the service provider, we should enforce in the interface the factory methods signature.
This is more or less what I have in mind:
Please note how I not used static methods. Refers to #5 for more info on the reason why.
I think that, even if not ideal, this is a probably acceptable trade off and reduce the "fragility" of current implementation.
Another variant could be that
servicesMap
, could just return a list of services names, (no information on service types). This would simplify the interface. In that caseservicesMap
should be renamed.Implementation example:
Note how in a real world library containing more service providers,
servicesMap
,createService
andcheckService
could be implemented in a trait or in an abstract superclass, so that each provider class could just define ownMAP
constant and the needed private factory methods.It means the code necessary would be very similar on current implementation (RAD not affected) but this implementation is capable to enforce factory method signature and returned type.
Among other things, this has the huge benefit that
$container->get('service')
is enforced to return a predictable type (and an IDE supporting the standard could take advantage of it).Finally, if this implementation will be take into consideration, we should probably also take into consideration to provide two exception classes, one to be thrown when
createService
receives a non provided service, and one to be thrown when the service type does not match the expected type. As alternative, one exception class with 2 different status codes (provided as class constants) should do.The text was updated successfully, but these errors were encountered: