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

Adding an instance of a class which has an __invoke method #113

Closed
willemwollebrants opened this Issue Jul 7, 2016 · 9 comments

Comments

Projects
None yet
6 participants
@willemwollebrants
Copy link

willemwollebrants commented Jul 7, 2016

I'm not sure if this is expected behavior or not, but here goes:

When you add an instance of a class which has an __invoke method, the DefinitionFactory sees this as a callable, so it gets added as a CallableDefinition. As a consequence of this, when you try to get the $concrete it does not return the instance, but whatever the __invoke method returns.

Here's an example to make it a bit clearer

I think this might be a bug, because changing the innards of a class should probably not change what the container returns. If my thinking is correct, the fix would probably be handling objects which are not instances of Callable like any other undefineable item and just add them as an arbitrary value/instance (= like here.

I'm more than willing to PR this, but I'd rather make sure this is indeed a bug and not by design, because I can imagine being able to use __invoke has its uses too.

@philipobenito

This comment has been minimized.

Copy link
Member

philipobenito commented Jul 8, 2016

@willemwollebrants yes this is a bug/oversight, the decision was made to handle is_callable classes in this way to allow for some nice organisation, however, as you point out it should not be to the detriment of a class that happens to have an __invoke method. Regardless of whether it is classified as a bug or not, this is unfortunately a BC break to fix and would have to aim at v3.0.0, there are a couple of smaller changes that are planned that also need to target v3.0.0 so I can either pick it up with those or I'd be happy to look at a PR to bring in.

@khelle

This comment has been minimized.

Copy link

khelle commented Aug 4, 2016

I believe it should be marked as bug, because the behaviour right now is inconsistent when using autowiring. Trying to make object from class that has __invoke method from container with autowiring treats the constructed object as object. On the other hand if concrete definition is provided, then container treats it as callable, meaning result of get() method on such classes is nondeterministic.

@philipobenito

This comment has been minimized.

Copy link
Member

philipobenito commented Mar 31, 2017

@hannesvdvreken @localheinz @khelle I'm developing 3.0.0 right now and struggling to come up with a good solution to this. Any thoughts?

My only clean solution is ignoring classes implementing __invoke as factories completely but I'd rather not do that.

@philipobenito

This comment has been minimized.

Copy link
Member

philipobenito commented Apr 1, 2017

Handled in 421d774 for release in 3.0.0.

Provided ClassName argument object to ensure that any class wrapped will be resolved as a class and not a callable.

@hannesvdvreken

This comment has been minimized.

Copy link
Contributor

hannesvdvreken commented Apr 3, 2017

It's an exceptional case, and there is a simple way of resolving an invokable object.
So I wouldn't consider it a bug, and won't spend time/lines of code to make a BC change for this small case.

@philipobenito

This comment has been minimized.

Copy link
Member

philipobenito commented Apr 3, 2017

I think the solution I've implemented works for this, won't be changing default behaviour though.

@hannesvdvreken

This comment has been minimized.

Copy link
Contributor

hannesvdvreken commented Apr 3, 2017

👍

@SammyK

This comment has been minimized.

Copy link
Member

SammyK commented Apr 24, 2018

FWIW, I was having this same issue in league/container version 2.4.1 when trying to register league/commonmark (0.17.5) into the container since League\CommonMark\CommonMarkConverter has an __invoke() method.

<?php

namespace Foo\Markdown;

use League\CommonMark\CommonMarkConverter;
use League\Container\ServiceProvider\AbstractServiceProvider;

class CommonMarkProvider extends AbstractServiceProvider
{
    protected $provides = [
        CommonMarkConverter::class,
    ];

    public function register(): void
    {
        $converter = new CommonMarkConverter;
        $this->getContainer()->share(CommonMarkConverter::class, $converter);
    }
}

Fails with error:

PHP Fatal error:  Uncaught ArgumentCountError: Too few arguments to function League\CommonMark\Converter::__invoke(), 0 passed and exactly 1 expected in /var/www/html/vendor/league/commonmark/src/Converter.php:73

My workaround was to make a wrapper for CommonMarkConverter and register the wrapper in the container:

<?php

namespace Foo\Markdown;

use League\CommonMark\CommonMarkConverter;

class CommonMarkWrapper
{
    private $converter;

    public function __construct()
    {
        $this->converter = new CommonMarkConverter;
    }

    public function toHtml(string $markdown): string
    {
        return $this->converter->convertToHtml($markdown);
    }
}

I hope that helps others who run into this issue. :)

@localheinz

This comment has been minimized.

Copy link
Contributor

localheinz commented Apr 24, 2018

@SammyK

I had exactly (!) the same issue a while ago.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment