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

Is this really necessary? #16

Closed
gmazzap opened this issue Mar 24, 2016 · 8 comments
Closed

Is this really necessary? #16

gmazzap opened this issue Mar 24, 2016 · 8 comments

Comments

@gmazzap
Copy link

@gmazzap gmazzap commented Mar 24, 2016

This want to be a fully constructive discussion, so first of all, sorry for the silly issue title: given the necessity to be concise and my poor english, I could not found anything better.

Some concerns about the idea...

After some discussions and thought, I am very convinced that standardize service providers is not a good idea at all.

Let me explain.

The stated aim of this standard is to avoid the ploriferation of framework-specific "providers" bundle for framework agnostic libraries.

The problem I see there is that those "providers" are poisonous packages by themself.

I want to use the same example used somewhere else in this repo: https://github.com/thephpleague?utf8=%E2%9C%93&query=glide

The link shows 5 different packages to support specific frameworks. Have you looked at the code of any of them? They are just 1 class with 1 method, most of the times very few lines.

We are talking of packages where the entire code is something like

Introducing an external dependency for things like those are very poor practice IMO.

The recent "disaster" with NPM should have taught all of us, that even if the rise of dependency manager make very easy to pull ready-made stuff from somewhere, we should think more than once before introduce a "require" line in our system.

I appreciate the fact that this standard want to eradicate the need those kind of packages, but that need is a very false need. No one really needs a package for 1 liner.

And if we need a standard to avoid writing 5 (at maximum) lines of code I think we have a problem.

In the mix we should consider that this standard will be shipped as an external dependency to be added to projects...

I really hope that this will be kept separate from ContainerInterface and that one will not require the other. In this way who, like me (assuming that someone agree with me, which is not sure at all), likes the ContainerInterface but not this, could happily use the one without the other.

Some concerns about the implementation...

I have some concerns regarding the current implementation as well.

I read in the fact that the stateless methods were chosen to guarantee stateless containers.

My question is: how that is possible? Static methods are called on the provider, so the fact they are static means nothing for the Container. Moreover, static methods does not guarantee that even the Provider is stateless.

class Provider implements ServiceProvider {

    private static $staticEvil;   

    private static $instance;   

    private $evil;

    public function __construct($state)
    {
        $this->evil =  $state;     
    }

    private function aDynamicMethod(ContainerInterface $container)
    {
        return new SomeService( $container->get( 'someDep' ) );
    }

    public static function getServices()
    {
        return [ 'myService' => 'getMyService' ];
    }

    public static function getMyService(ContainerInterface $container)
    {
        self::$staticEvil or self::$staticEvil = 'some static state';
        self::$instance or self::$instance = new static('some dynamic state');

        return self::$instance->aDynamicMethod($container);
    }
}

As you can see there no warranty provider is stateless. And from the code above we do know nothing about the container: it could be be stateless or stateful. How we could ever know?

On the contrary, I somewhat think that static methods promote the usage of static state for Providers that is worse than dynamic state.

The other argument used in favor of static methods is that they are easier to call. Why?

See following two fictional implementation:

$providers = [
   A\Provider::class,
   B\Provider::class
];

$container = new Container();

foreach($providers as $provider) {
  $services = call_user_func($provider, 'getServices');
  array_walk($services, function($method, $name, $container) use($provider) {
     $cb = [$provider, $name];
     // let's assume a Container who implement `set` method
     is_callable($cb) and $container->set($name, $cb($container));
  }, $container);
}

VS

$providers = [
   new A\Provider(),
   new B\Provider(),
];

$container = new Container();

foreach($providers as $provider) {
  $services = $provider->getServices();
  array_walk($services, function($method, $name, $container) use($provider) {
     $cb = [$provider, $name];
     // let's assume a Container who implement `set` method
     is_callable($cb) and $container->set($name, $cb($container));
  }, $container);
}

Which is the difference? In which way the former is better than the latter? How the former guarantees any assumption on the Container state?

On the contrary, if I have to think about a difference, I think the latter is better.

Reason is that specific implementations of container could introduce methods to register services that accept a provider as argument, taking advantage of type hint:

class Container implements ContainerInterface
{
    public function register(ServiceProvider $provider)
    {
     $services = $provider->getServices();
     // register services here...
    }
}

The static counterpart:

class Container implements ContainerInterface
{
    public function register($provider)
    {
      if (!is_subclass_of($provider, ServiceProvider::class, true)) {
         // throw exception here
      }
      $cb = [get_class($provider), 'getServices'];
      $services = $cb();
      // register services here...
    }
}

looks quite nasty...

Moreover, in both cases, due to a limitation of PHP, we are not able to check what getServices() actually returns: we can check [$provider, $name] is callable, but how can we check callback signature? This can only be checked on runtime using reflections (and no one should do that) and can't be enforced by the interface in any way.

Actually, the only possible enforcement would be via comments, something like:

interface ServiceProvider {

    /**
     * Provides factory method names.
     *
     * MUST return an array where item keys are service names (as strings)
     * and values are static method names in the current class, aka "factory methods".
     *
     * Factory methods signature MUST be:
     * `public static methodName(ContainerInterface $container, callable $previous = null)`
     *
     * @return strings[]
     */
     public static function getServices();

}

Am I the only one who this interface looks very fragile? All the behavior is enforced via comments and even in PHP 7, where it is possible to enforce the return type, there will be no way to enforce what the returned array will actually contain.

Before I finish let me shoot a couple of questions.

Per my understanding, service providers are just a way to set services "in bulk" in the container. If there were very good reasons (and there were) to don't provide services "setters" in the ContainerInterface, why those reasons are not valid for service providers?

And finally, there's really the need of a standard to write packages that in most cases will just be few liners?

Conclusion

Even if this is critic about the what is being done here, I want to make explicit (if there is the need) that it is a critic to code and to code concepts, and not to people or their work.

I have very deep respect for the people who are working here, and I'm not talking about humans (who deserve respect in any case) but about coders.

I hope you understand that I put my time and effort to make this issue a constructive discussion, and maybe help to make this standard better, if - as I guess - I can't persuade you to just abandon it :D

@moufmouf
Copy link
Contributor

@moufmouf moufmouf commented Mar 25, 2016

Hey Giuseppe,

Thanks a lot for the constructive feedback.

There are really 2 different questions you are asking. One regarding the necessity of having service providers and one regarding the way we implemented it.

Let's start with the necessity of service providers.

First, you are not the only one thinking that these kind of packages are not necessary. Usually, people used to "autowiring containers" are reluctant to using service providers (because the container is supposed to create the services "auto-magically").

That being said, there are a ton of those packages out there, so we just can't say they are useless and deny their existence. Also, if you followed the discussion about PSR-11 and the ContainerInterface, you probably noticed that there has been some push back from a part of the community. The entrance vote was passed, but with a faire number of -1, and must of the people voting +1 did so expressing a need to work on PSR-11 draft. Negative votes were about saying that we should standardize how we put things in containers rather than how we fetch entries from it. So this debate is not over yet. I'm pretty sure a vote on PSR-11 would not pass until we sort out both issues. This is what we are trying to do with service-providers.

I really hope that this will be kept separate from ContainerInterface and that one will not require the other. In this way who, like me (assuming that someone agree with me, which is not sure at all), likes the ContainerInterface but not this, could happily use the one without the other.

Duly noted. As of today, you will notice that there are really 2 separate packages: container-interop/service-provider and container-interop/container-interop. If we manage to make this a PSR, maybe we need to keep those 2 separate packages.

About the details of the implementation

I read in the fact that the stateless methods were chosen to guarantee stateless containers.
My question is: how that is possible? Static methods are called on the provider, so the fact they are static means nothing for the Container. Moreover, static methods does not guarantee that even the Provider is stateless.

It is true that in the FAQ of the README, we say:

Using static methods somewhat guarantees everything to be stateless, which cannot be guaranteed otherwise.

This is clearly false as you show in your example.

@mnapoli : we need to rewrite this part of the FAQ.

The idea was really that any method providing a service SHOULD do so in a stateless manner. The result of the method should only depend on the arguments of the method and nothing else.
In that regard, the fact that the method is static conveys the idea that the method should be stateless. You are right saying that there is no way we can "guarantee" the method to be stateless (the developer could always use globals or whatever...)

Now, here are the advantages I see to using static methods:

  • as I say, it conveys the idea that the method should be stateless
  • also, it is way easier to implement adapters for frameworks with compiled containers. For instance, it was kind of easy to add a bridge to Symfony because of this.
  • finally, we can easily use discovery mechanisms on classes to automatically detect the service providers (here I think about Puli integration). Doing so with instances rather than classes is way harder.

That being said, your comments about the cons of static methods are perfectly valid. There have been other comments about it here: #5. This definitely needs to be discussed a bit more. Maybe at some point we should write a big table with pros and cons :)

I hope you understand that I put my time and effort to make this issue a constructive discussion, and maybe help to make this standard better, if - as I guess - I can't persuade you to just abandon it :D

Thanks a lot for the constructive feedback!

@mnapoli
Copy link
Member

@mnapoli mnapoli commented Mar 25, 2016

Hi, thanks for the questions and answers. I don't have a lot of time to answer everything like I would like to, but I'll focus on a few things for now:

I want to use the same example used somewhere else in this repo: https://github.com/thephpleague?utf8=%E2%9C%93&query=glide

The link shows 5 different packages to support specific frameworks. Have you looked at the code of any of them? They are just 1 class with 1 method, most of the times very few lines.

Thanks, this is indeed a bad example. We should find another example to link to. However I disagree with the conclusions you draw from this bad example. The example does not represent accurately all examples we could find out there.

Here is a counter example: http://knpbundles.com lists 2788 Symfony bundles. I don't think it's fair to say they are all useless.

Using static methods somewhat guarantees everything to be stateless, which cannot be guaranteed otherwise.

This is clearly false as you show in your example.

@mnapoli : we need to rewrite this part of the FAQ.

I'm sorry but come on… It's possible to return false even though the phpdoc says @return array. It's possible to access private properties using reflection. It's possible to hack anything using Runkit.

That doesn't mean that private is useless or that phpdoc is useless because users can hack around it.

Maybe the word "guarantees" is too strong and can be improved, but let's use common sense please… If someone wants to do something wrong, there's a lot of possibilities.

Without static it's perfectly fine for developers to make their service providers stateful. That's what instances are for: having a scope and properties/data inside that scope.

If we think service providers should be stateless for interoperability reasons, we make it explicit they shouldn't be instances by requiring methods to be static. It's now obvious what the intent is, and it's harder to get it wrong.


By the way we can also discuss separate concerns in separate threads if it's easier.

Cheers!

@mnapoli
Copy link
Member

@mnapoli mnapoli commented Mar 25, 2016

I agree though that being able to type-hint against ServiceProvider is a plus. However I don't think it counterbalances the gain we have with static. FYI I'm not opposed to removing static, I just want to make the right decision based on arguments.

@gmazzap
Copy link
Author

@gmazzap gmazzap commented Mar 25, 2016

On the idea, again...

Let's start with the necessity of service providers.

@moufmouf You don't have to convince me of necessity of service providers. I like them. For instance, in this library I wrote you can find 6 of them.

What I feel is bad is service providers as external dependencies. Most of the times, a service provider is a class with 1 method and 0-to-10 lines of code.

@mnapoli Surely sometimes there might be more complex and more useful providers, but I think that's the exception, not the rule.

The main reason why I'd rather don't see this a part of a PSR is that I'm absolutely sure that this will bring to a proliferation of one liner packages, and to the fact that we, as PHP developers, will accept that as a current best practice that worth to be added as PSR.

My (questionable) point can be summarized like this:

  • service providers are a good thing
  • service providers need to set "things" in a container
  • the container interface we agreed to have doesn't have methods to set "things"
  • service providers are all about setting services, and without those container "setters" methods every attempt to make this standard doable will be hackish and fragile... (and doesn't worth to be added as a community standard)

If we give up to have a standard for service providers means that client code (i.e. users) will have to write service providers by themself (using container-specific interface). Which IMO is not a big deal: I mean, you know already that story of 10 lines of code.

Are there cases in which a service provider do something that worth to be added as separate dependency? Fine, it means that library author (or someone else) can create framework specific adapters. I am very sure that moderns library that needs complex service providers will be so rare that this will no be an issue. Or anyway IMO an issue much smaller than an uncontrolled (and actually promoted as best practice) proliferation of 1 liners framework agnostic providers packages.

The cons without standardizing providers is that users will have to write their service providers by themeself based on the container of choice (that will ship with a container-specific provider interface). But, again, this is not a big deal. For nth time: most of the times it's a single class, 1method, 10 lines of code. We don't need a standard to avoid that.

The pros with standardizing providers is that libraries can write one single provider for all frameworks. This save to write "some" work. No need to repeat how much.

However, the cons standardizing providers is that we will have a lot few lines, making-nothing, packages. Moreover, (but this is more personal) we will promote as a community standard something that is hackish and fragile.

My rant about the idea stops here.

On statics & interfaces

Without static it's perfectly fine for developers to make their service providers stateful. That's what instances are for: having a scope and properties/data inside that scope.

I disagree. To me, forcing static methods will be perfectly fine for developers to make their service providers make use of static state. Simply put: if you add an interface with a static method you are actually promoting the usage of self to store state in place of $this. And if the latter is bad, the former is worse.

Moreover, talking about interfaces, they are an entirely behavioral entity. They define a contract on a behavior that can be polymorphically implemented by objects. Interfaces have nothing to do with "properties" or "data". This is why you can't define properties in a interface. And this is why a lot of languages don't allow static methods to be defined in a interface: "objects" and "classes" are not the same thing. Interfaces should be all about objects, and nothing about classes. And it's not a fortuity that a modern language like Go has the concept of interfaces without having the concept of classes.

My rant about software design stops here.

On implemenation, again...

It's possible to return false even though the phpdoc says @return array. It's possible to access private properties using reflection. It's possible to hack anything using Runkit.
That doesn't mean that private is useless or that phpdoc is useless because users can hack around

Sure thing. The problem is that being (or wanting to be) part of a standard being able to formally enforce, as much as possible, expected behavior is pivotal.

Most of us (I think all of us in this discussion) were happy of the fact that PHP introduced type return and scalar type hints. Why? Because enforcing types is a way to make code cleaner, more solid and more "intention revealing", so easier to read, easier to analyze and profile, easier to debug, easier to learn.

I think that target of a standard should be try to enforce the expected behavior as strongly as possible.

For instance, if a standard expects a method I think is a good thing if it enforces its signature (if not the return type because of language limitation).
This is usually how standards use interfaces, but current implementation of this standard not only isn't able to enforce the signature of factory methods, it is not even able to allow type enforcement of the main object subject of the standard, because it promotes statics.

IMO this makes the standard fragile, and this fragility makes it a bad standard. Surely people can do crazy thing with good standard as well, but that's not a good reason to don't care about standards quality.

I have to say also that I totally get that static make current implementation easier. But easier and good are not the same thing. And, for the record, easier and simple are not the same thing.

I hope you understand that I don't think the standard is bad because you are not good at coding, or anything, instead I think that, self-citing me:

service providers are all about setting services, and without those container "setters" methods every attempt to make this standard doable will be hackish and fragile.

In other words, I'm pretty sure that it's pretty much impossible to write a standard interface that will not suffer of this "fragility" or hackish design.


A "crazy" proposal

Note: it's friday evening, it's half holiday, and I'm still a bit under the effect of anesthetics, so if I'll write crazy things please forgive me.


Do you really want to standardize service providers? What about a json file?

{
  "schema":   "https:/some-url-here.org/schema#",
  "services": {
    "logger":     {
      "factory":   "App\\Providers\\LoggerProvider::createLogger",
      "arguments": [],
      "type":      "Psr\\Log\\LoggerInterface",
      "meta":      "JSON has no comments, so this could be a useful"
    },
    "middleware": {
      "factory":   "App\\Providers\\Middleware::createMiddleware",
      "arguments": [
        "@logger",
        "string"
      ],
      "type":      "callable"
    }
  }
}

Note how, for each service, "type" (that is cactory return type) can be a scalar, a class or an interface; and how "arguments" (that are arguments to be passed to factory), can contain names of services to be resolved in the container. (This needs some covenant, I used @ to be prepended to argument type, but it's just an idea).

The obvious cons in this approach is performance, but I'm pretty sure that:

  • each container (or each framework) can easily use some strategy for cache (or someone could write a framework-agnostic package for it :P)
  • containers that use uncached "autowiring" based on reflections will not be that much faster

However there are different pros:

  • factories are always callbacks. Since they are in a JSON file, they can only be static or function names. This is the approach Composer is using for its "scripts". Since it isn't PHP, there are no way to use closures of dynamic methods, this avoid any problem you wrote in FAQ;
  • factory methods can be anywhere, no need to be placed in a specific class;
  • it is possible to warranty the signature of the methods, including arguments that must be resolved in the container
  • it is possible to enforce the return type of factories, even with PHP 5;
  • "self discovering" performance will improve (by order of magnitude, I guess) because no need to "selfscover" anything: everything is very explicit
  • it is a godsend for compiled containers, and it practically means that frameworks (or users) can make any container compiled, in a container-agnostic way
  • the easiness to validate against a JSON schema (and the fact that related schema is declared in document) make validation very easy
  • it is parseable by different languages (e.g used in automation scripts) and even by javascript (:trollface:)
moufmouf added a commit that referenced this issue Mar 25, 2016
Following discussion in #16, I've tried to emphasize the fact that we expect factory methods to be stateless. I've also added a few words on what we mean by "stateless". Let me know if you think this is clear enough or if this still needs some work.
@mnapoli
Copy link
Member

@mnapoli mnapoli commented Mar 25, 2016

What I feel is bad is service providers as external dependencies. Most of the times, a service provider is a class with 1 method and 0-to-10 lines of code.

Why would service providers be in a separate package? I don't see a reason for that since there would be a standard, there would be nothing to decouple with so no reason to have a separate package.

The main reason why I'd rather don't see this a part of a PSR is that I'm absolutely sure that this will bring to a proliferation of one liner packages

Problem solved then :)

service providers are all about setting services, and without those container "setters" methods every attempt to make this standard doable will be hackish and fragile...

This is inversion of control.

Instead of service providers telling the container to register something (which isn't possible as said above), containers ask service providers what to register. That is compatible with all containers since it's just implementation detail: they have to support a standard format of service providers.

client code (i.e. users) will have to write service providers by themself

There are numerous packages out there (modules, bundles, etc.) proving that there is a need. Maybe you don't need them, and I respect that (I tend to avoid them too when I can). But the PHP ecosystem today is as it is, it's a fact, modules are very popular. Keep in mind many developers need to code fast (RAD) or others are not skilled enough to architect their application. I guess you could even be fine without using frameworks, but most developers still need it and so that doesn't make them bad. This is also a problem because it creates rifts in communities (e.g. Symfony/Laravel/ZF/…) because some modules/bundles are only created for specific frameworks. We need to improve that.


Do you really want to standardize service providers? What about a json file?

Please see https://github.com/container-interop/service-provider#background Have a look also at https://github.com/container-interop/fig-standards/blob/container-configuration/proposed/container-configuration-meta.md and container-interop/fig-standards#10 That's some background on why we didn't go that way.

Also I'm picking at you for fun: JSON files dont call setters on containers (like service providers), you don't think it's bad? ;)


Could we split sub-topics in separate issues? Wall of texts take a lot of time to answer and write, smaller separated comments will be easier to follow and discuss.

@gmazzap
Copy link
Author

@gmazzap gmazzap commented Mar 25, 2016

Why would service providers be in a separate package? I don't see a reason for that since there would be a standard, there would be nothing to decouple with so no reason to have a separate package.

Because hundreds of libraries will not implement the standard as soon as it is approved. And before a large number of libraries will, long time will pass. In meantime, you can be sure (I am) that someone will release service provider packages for anything that don't do it. If I'll be wrong on this I'm ready to pay you a beer the 1st time I'll be lucky to meet you ;)

This is inversion of control.

Actually it would be IoC if:

  • Container tells providers to register services (control is moved to providers)
  • Providers do the job (they take control)

This implementation is:

  • Container ask providers what should be done (control is moved from Container to Providers)
  • Providers answer with some methods names (control is moved back to Container, that can do anything with received methods names)
  • The container calls the factory methods on the providers (control is set back again to providers, that instantiate services)
  • Built services are finally "registered" by the container in the container, so control is back again to it.

This sounds to me more a "confusion of control" . On the contrary, it seems to me a perfect negation of the "tell don't ask" paradigm.

There are numerous packages out there....

If this is becoming a RAD VS quality code, I'm giving up. Because I am for quality, and I know I can't win.

Also I'm picking at you for fun: JSON files dont call setters on containers (like service providers), you don't think it's bad? ;)

Sure I am. I wrote that coding this in a good way is just not possible. This is why I said "Do you really want standardize service providers?": because I am still convinced that would be better to just don't.

Could we split sub-topics in separate issues? Wall of texts take a lot of time to answer and write, smaller separated comments will be easier to follow and discuss.

As I said, I'm giving up with this discussion. And if the only "pro-active" proposal I made was already discussed and refused, I don't think I can contribute more here. But if you think this can be useful for others, I can split my text in smaller parts, at least I can say that was my contribute.

@mnapoli
Copy link
Member

@mnapoli mnapoli commented Mar 27, 2016

Because hundreds of libraries will not implement the standard as soon as it is approved. And before a large number of libraries will, long time will pass.

PSR-3 was immediately implemented. PSR-7 was implemented and used before even voted. PSR-6 was part of Symfony and other implementations the day after being accepted. Projects are implementing PSR-11 through container-interop today. I am not worried about this like you are.

Actually it would be IoC if:

I'm saying it's an inversion from what you are suggesting. You are saying it's an inversion from what we are suggesting. I think we are running in circles.

Let's center the discussion back on what's important: what can work, and what can't. What you suggest cannot work. What we suggest works. Do you disagree with that?

If this is becoming a RAD VS quality code, I'm giving up. Because I am for quality, and I know I can't win.

I don't think both are mutually exclusive. Also I don't understand why you care about something and try to stop it from happening if you will not be using it as you said before? If you don't like using modules, then why do you want to deprive the world from a better module solution than what exists today? What solutions would you offer to people wanting to build application rapidly, or to developers not skilled enough to implement everything themselves? Please understand I'm not trying to put you in a corner, I really want to understand your POV because I believe it is interesting, it's different, and I think understanding it can contribute to the project in itself (we need to understand what people are looking for).

And if the only "pro-active" proposal I made was already discussed and refused

It wasn't really refused, it was just deemed less interesting based on the data I have linked to, se we are focusing our work on this solution for now. If you think we missed some pros or cons, please send a pull request or open an issue? If the conclusions change because of something we missed we could discuss this format again.

@gmazzap
Copy link
Author

@gmazzap gmazzap commented Mar 27, 2016

On a second think, considering pros and cons of JSON (or XML) approach I think it does not worth the complexity it adds.

By the way, for me this issue can be closed. I'm going to add some thoughts on #5 and open a new issue to try to improve current implementation.

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

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.