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

Feature request: providers #147

Closed
frederikbosch opened this issue Mar 20, 2017 · 16 comments
Closed

Feature request: providers #147

frederikbosch opened this issue Mar 20, 2017 · 16 comments

Comments

@frederikbosch
Copy link
Contributor

At this moment, I am missing an abstraction in Aura.Di. That is the ability to provide an object with features coming from another service. Mostly those objects are collections, for example to provide a list of commands to a console application. This problem is mainly caused due to the fact that we have more than one location that is able to modify the container content (plugins).

My current solution looks as follows:

// primary container constructor
$container->set('console.commands', \new ArrayObject());
$container->set('console.application', $container->lazy(function () use ($container) {
  $application = new ConsoleApplication();
  foreach ($container->get('console.commands') as $commandName) {
    $application->addCommand($container->get($commandName));
  }
  return $application;
}));

// in some other container modifier class
$container->get('console.commands')->append('console.command.my.command');
$container->set('console.command.my.command', $container->lazyNew(MyCommand::class));

I wish this package could be enhanced with an easier solution. And a solution where I do not need closures, because that prevents the container from being serialized. An example solution looks as follows.

// primary container constructor
$container->set('console.commands', \new ArrayObject());
$container->set('console.application', $container->lazyNew(ConsoleApplication::class));

// in some other container modifier class
$container->providers[ConsoleApplication::class][] = $container->lazyNew(ConsoleApplicationProvider::class));

// in the provider
class ConsoleApplicationProvider implements ProviderInterface {

   public function provider($container, $object) {
      $object->addCommand($container->newInstance(MyCommand::class));
   }

}

While this has similarities with a factory, it is not completely the same. Hopefully, this can be taken into consideration. When the feedback is positive, I am happy to create a PR for it.

@harikt
Copy link
Member

harikt commented Mar 20, 2017

Hey @frederikbosch ,

Let me give a wild guess that you are using symfony/console. If so, I have another thought

$container->set('console.commands', \new ArrayObject());
$container->set('console.application', $container->lazyNew(ConsoleApplication::class));
$container->setters[ConsoleApplication::class]['addCommands'] = $container->lazyGet('console.commands');

$application = $container->get('console.application');

I haven't tested it, I guess this should work.

But, I see this

// in some other container modifier class
$container->get('console.commands')->append('console.command.my.command');

will not work, instead you have

class  Common extends ContainerConfig
{
    public function modify($container)
    {
        $application = $container->get('console.application');
        $application->addCommand($container->get(MyCommand::class));
        // or
        $application->addCommand($container->newInstance(MyCommand::class));
    }
}

Thoughts ?

@jakejohns
Copy link
Member

@harikt I think you mean $application = $container->get('console.application');.
eg. "GET", not "SET".

@harikt
Copy link
Member

harikt commented Mar 20, 2017

@jakejohns exactly get . Thank you. Fixed.

@frederikbosch
Copy link
Contributor Author

frederikbosch commented Mar 21, 2017

@harikt Symfony Console is just an example. I dont even know if the API in the example is correct. I have two thoughts with your example. First, the objects will be loaded in memory even if I do not need them. In other words, they are not lazy. Furthermore, how do I call public functions with more than one argument to an object that has been instantiated in the container? A setter can only have one argument. So I cannot use a setter for the following: addExtension($name, $object).

My feature request basically enables the following functionality. Through providers one can modify an object (through its public API) after it is instantiated in the container.

A list of advantages:

  1. One can modify an object after it has been instantiated.
  2. One does not have to get an object from the container when the object is not needed, and as such it does not have to be instantiated. It is completely lazy.
  3. One is not limited to ->setter calls and therefore not limited to a single argument for a function call
  4. One does not need closures to register the object modifiers, which prevents the container from being serialized.
  5. The provider API provides simplicity to modify an object

@jakejohns
Copy link
Member

Possibly relevant issue I raised previously, #122

Punchline from @pmjones :

[...] you can still wrap the creation logic in a factory, and pass an array of params to the factory, and let the factory apply those params to the methods to be called.

@frederikbosch
Copy link
Contributor Author

@jakejohns Thanks for that. It seems to me that you did quite a similar request. Hopefully a provider solution makes sense and are @pmjones and @harikt willing to consider as a feature. If so, again, I am happy to push PRs.

@jakejohns
Copy link
Member

@frederikbosch After the brief conversation with @pmjones I was won over to the perspective that, at least for my needs, I didnt think it belonged in the DI package, but rather in my own code base for the specific implementation I needed.

What is your objection to using a "Factory" to which you pass the "Commands"?

Off the top of my head, something like (hastily, sloppily, untestedly ):

<?php
// @codingStandardsIgnoreFile


/**
 * Offset the complicated creation to a seperate custom factory, rather than try
 * to get the DI to know how to build specific complex things.
 */
class AppFactory
{
    protected $resolver;

    public function __construct(callable $resolver)
    {
        $this->resolver = $resolver;
    }

    public function newInstance($commands)
    {
        $application = new ConsoleApplication();
        foreach ($commands as $name => $command) {
            $application->addCommand($name, $this->newCommandFactory($command));
        }
        return $application;
    }

    // Are you trying to pass 'Lazy' callable type factory things to the application?
    // If not, this is unnecessary
    protected function newCommandFactory($command)
    {
        $resolver = $this->resolver;
        return function() use ($command, $resolver) {
            return $resolver($command);
        }
    }
}


class Config extends ContainerConfig
{

    public function define($di)
    {
        // Have the factory use that ResolutionHelper thing, maybe?
        $di->params[AppFactory::class]['resolver'] = $di->newResolutionHelper();

        // This could probably just be values
        $di->set('commands', \new ArrayObject());

        // Make the factory a service?
        $di->set('factory', $di->lazyNew(AppFactory::class));

        // use LazyGetCall on the factory
        $di->set(
            'application',
            $di->lazyGetCall(
                'factory', 'newInstance', $di->lazyGet('commands')
            )
        );

        // set up the commands as services
        $container->set('command.service.name', $container->lazyNew(MyCommand::class));
    }

    public function modify($di)
    {
        // Add all yr commands
        $container->get('commands')->offsetSet(
            'name',
            'command.service.name'
        );
    }
}

@frederikbosch
Copy link
Contributor Author

@jakejohns Many thanks for clearing up! This example does achieve what I want to. I could go for that. However, I do think that providers are API-wise a more elegant solution to the problem that at least two people had.

@pmjones @harikt Are you willing to consider a 'better' API for the problem? If not, I will let it go and go for the solution as suggested by @jakejohns.

@harikt
Copy link
Member

harikt commented Mar 23, 2017

@frederikbosch if you are in a hurry I would suggest you to go with the approach.

I am not sure, but I don't have any objections also. But the final word is from @pmjones . So you may want to wait for him.

@djmattyg007
Copy link
Contributor

I highly recommend the webmozart/console package. It's a wrapper around Symfony Console that removes the need to instantiate all of your command classes just to bootstrap your console application by divorcing the command configuration from the code being run.

@jakejohns
Copy link
Member

This is getting off topic, but fwiw: simple php console app thing

@frederikbosch
Copy link
Contributor Author

@jakejohns @djmattyg007 Thanks for your suggestions. Console was merely an example. Maybe this should be explained as a shortage of setters and factory.

With setters you can only use 1 argument. What if the object is immutable and returns a clone? That is not possible with setters. While with factory one can only change an object on instantiation. If you are using an heavily plugin based system, like I am using, then this can only be solved with lists (array / ArrayObject). Then code complexity increases a lot.

With providers, the object can be modified after instantiation. Any plugins modifying the container can add its own providers for the specific objects.

@pmjones
Copy link
Member

pmjones commented May 25, 2017

Hey @frederikbosch -- I get where you're coming from on this. The main problem, as I note in #122 (comment), is that coming up with a widely-applicable configuration process for that kind of thing is pretty tough. For example, re: immutables, how do you specify (in a generic way) that a method call result should be retained in place of the object it was called on? Tricky, tricky.

Now, while I'm still of the opinion that a factory is the right thing here, I am open to revising my position. ;-) So I'll be happy to review any PR you want to send along that "scratches your itch" regarding this issue.

@andrewshell
Copy link
Contributor

@frederikbosch Do you think #151 might solve some of your issues? This is the exact use case I was trying to solve. I wanted different containers to be able to register the commands they provide.

@frederikbosch
Copy link
Contributor Author

@andrewshell Not quite, but it helps! I am planning to start working on this in a few weeks.

@frederikbosch
Copy link
Contributor Author

Closing, I will create a PR for mutators any time soon.

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

No branches or pull requests

6 participants