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

container->has(..) false positive #847

Open
palansher opened this issue Feb 4, 2023 · 7 comments
Open

container->has(..) false positive #847

palansher opened this issue Feb 4, 2023 · 7 comments

Comments

@palansher
Copy link

Hello!

php-di/php-di 6.4.0
Ubuntu 20.04
PHP 8.1

\dd($this->container->has(TopAccount::class));

This result is true and this claims that an object with id 'Gt\PaymentToDriver\TopAccount' is in container:

image

\dd(TopAccount::class); gets:

image

But if you inspect container object in scope of same class's method, there is no object with id "Gt\PaymentToDriver\TopAccount"

\dd($this->container);

image

As result of this false positive, when I try get this item

if ($this->container->has(TopAccount::class)) {
                $ta=$this->container->get(TopAccount::class);             
            }

I got unexpected exception, instead of the object kept in container:

[2023-02-04T04:04:37.191379+03:00] CommonErrors.ERROR: DI\Definition\Exception\InvalidDefinition: Entry "Gt\PaymentToDriver\TopAccount" cannot be resolved: Parameter $id of __construct() has no value defined or guessable
Full definition:
Object (
    class = Gt\PaymentToDriver\TopAccount
    lazy = false
    __construct(
        $id = #UNDEFINED#
        $driver = get(Gt\Model\Gk\Driver\Entity\Driver\Driver)
        $top = get(Gt\Taxi\Operator\TopInterface)
        $base = get(Gt\PaymentToDriver\Base)
        $topAccountDriverId = #UNDEFINED#
        $topAccountDriverName = #UNDEFINED#
        $topLink = get(Gt\PaymentToDriver\TopLink)
    )
) in /app/vendor/php-di/php-di/src/Definition/Exception/InvalidDefinition.php:19
Stack trace:
#0 /app/vendor/php-di/php-di/src/Definition/Resolver/ObjectCreator.php(156): DI\Definition\Exception\InvalidDefinition::create(Object(DI\Definition\ObjectDefinition), 'Entry "Gt\\Payme...')
#1 /app/vendor/php-di/php-di/src/Definition/Resolver/ObjectCreator.php(71): DI\Definition\Resolver\ObjectCreator->createInstance(Object(DI\Definition\ObjectDefinition), Array)
#2 /app/vendor/php-di/php-di/src/Definition/Resolver/ResolverDispatcher.php(71): DI\Definition\Resolver\ObjectCreator->resolve(Object(DI\Definition\ObjectDefinition), Array)
#3 /app/vendor/php-di/php-di/src/Container.php(390): DI\Definition\Resolver\ResolverDispatcher->resolve(Object(DI\Definition\ObjectDefinition), Array)
#4 /app/vendor/php-di/php-di/src/Container.php(139): DI\Container->resolveDefinition(Object(DI\Definition\ObjectDefinition))
#5 /app/src/Model/Gk/PayToDriver/UseCase/AdminPay/ShowForm/Handler.php(44): DI\Container->get('Gt\\PaymentToDri...')
#6 /app/src/Admin/PaymentController.php(67): Gt\Model\Gk\PayToDriver\UseCase\AdminPay\ShowForm\Handler->handle(Object(Gt\Model\Gk\PayToDriver\UseCase\AdminPay\ShowForm\Command), Object(Gt\Model\Gk\PayToDriver\Entity\Payment\Payment))
#7 /app/src/Http/FrontController.php(122): Gt\Admin\PaymentController->handle(Object(Slim\Psr7\Request))
#8 /app/src/Http/FrontController.php(99): Gt\Http\FrontController->runAdminLegacyAction('payment')
#9 /app/public/admin.php(14): Gt\Http\FrontController->runAdminController()
#10 {main}  

So, my conclusion is:

$container->has(TopAccount::class) or the same: $container->has('Gt\PaymentToDriver\TopAccount'), returns false positive result.

Maybe there is some other method to ensure that there are no "Gt\PaymentToDriver\TopAccount" item in container?

@palansher
Copy link
Author

palansher commented Feb 4, 2023

Also noticed, that 'FetcheDefinitions" property of container has no "Gt\PaymentToDriver\TopAccount" key before ->has('Gt\PaymentToDriver\TopAccount'), and has it after such checking.

Maybe its normal. But my goal is to set the container entry IF it was NOT set before.
But this false positive by ->has() prevent this.

The ->getKnownEntryNames() does not contain 'Gt\PaymentToDriver\TopAccount', but has(Gt\PaymentToDriver\TopAccount) show True.

The ->debugEntry(TopAccount::class) shows:

Object (
    class = Gt\PaymentToDriver\TopAccount
    lazy = false
    __construct(
        $id = #UNDEFINED#
        $driver = get(Gt\Model\Gk\Driver\Entity\Driver\Driver)
        $top = get(Gt\Taxi\Operator\TopInterface)
        $base = get(Gt\PaymentToDriver\Base)
        $topAccountDriverId = #UNDEFINED#
        $topAccountDriverName = #UNDEFINED#
        $topLink = get(Gt\PaymentToDriver\TopLink)
    )
)

But I did not set 'Gt\PaymentToDriver\TopAccount' in container - by set() or thru container definitions.

@SvenRtbg
Copy link
Contributor

SvenRtbg commented Feb 4, 2023

The PSR container interface definition states: https://www.php-fig.org/psr/psr-11/#112-reading-from-a-container

has takes one unique parameter: an entry identifier, which MUST be a string. has MUST return true if an entry identifier is known to the container and false if it is not. If has($id) returns false, get($id) MUST throw a NotFoundExceptionInterface.

"Known to the container" is quite easy to determine if the container requires explicit definition of every entry it shall create, but the beauty of PHP-DI is the autowiring which enables it to create much more classes than are explicitly defined. Returning has() = false for something that hasn't been created yet by requesting it via get is violating the last sentence of the interface definition: If you ask the container if it can return you a class, and the answer is no, attempting to still try it is wrong.

I am quite convinced discussing the motivation for you to do what you intend would reveal that you try to configure the DI container "later", once some additional information has been gathered and now is supposed to be stored back into the container for future use. I personally would object such an approach, as my understanding of the role of a DI container is to be created/configured with some defineable set of configuration data (including strings for URLs, usernames, passwords), and then this container is not changing anymore, but only handing out objects to work on whatever task is to be done. If "dynamic" objects have to be created that depend on the actual data that is to be processed, this is not the task of a DI container, but should be done by some specific factory object - which itself may be injected into position by the DI container.

@palansher
Copy link
Author

palansher commented Feb 4, 2023

Good evening!

Thank you for explanation.

You are right, I try to use container as a cache to share the object between calls of different methods/procedures.
I agree with your conception of DI container, but must do it due lack of a cache like Redis :)

I will use strict string IDs for has() args instead of class strings, to be sure that IDs is surely is 'unknown' for container.

Besides of that, just a note:
I have read psr-11.. and still confusing why 'has()'=true does nor correspond lexically to method's name.
"has" means that container HAS and ABLE to return some data/object thru get('id') .
If container is 'know' about some class (thru class autoloading i.e.) but can't return it, such method must be called 'isAware()' :)

@palansher
Copy link
Author

So, currently it is no method to be sure that an object is retrievable by class string(ClassName::class). currently it is possible only by regular string. Unfortunately.

@SvenRtbg
Copy link
Contributor

Exactly. We are talking about two distinct things here. has() and get() are part of the PSR ContainerInterface, and they are read-only. The details about how to configure the container are intentionally omitted from the interface (i.e. there is no set() required to exist), and the expectations about how an implementation really reacts when executing has() are stated above, but are by no means exhaustive.

PHP-DI does attempt to verify with low effort whether or not the container knows about a requested class, however it does not do the full run of actually instantiating the object, as that would potentially be the full recursive autowiring required, which might be too much of a performance impact.

And I can see that returning true in your case is somewhat correct in terms of the promise associated by the interface definition: The class is somewhat "known to the container", the only thing missing is an entry for one constructur parameter definition. So it is something indirectly missing.

Using any other DI container that is forcing you to provide an explicit configuration for every class you want to fetch has a much easier time figuring out if an entry for something exists. PHP-DI would be forced to identify whether or not the whole potential recursive class tree is buildable - it's barely a matter of "obviously it could detect there is a string parameter missing on the top level, why not use that info to return false".

My recommendation would be to use a wrapper class for your missing info instead. Inject a class that can then be queried whether or not it already has your class, and can thruthfully answer yes or no, and then offer a method to inject a new instance into. That is offloading your requirement away from the container back into your own code's responsibility, it is explicit in a way that makes it obvious a value isn't present at the time the container is started, but has to be fetched later. Much like the PSR interfaces for request and response - they are also usually not created within the DI container, but a request factory is being fetched from the container in case a request has to be created.

@palansher
Copy link
Author

Thank you @SvenRtbg!

I am not such a qualified developer to discuss this subject with you detailed.
I will try to use your advices.

But, even after your answer I still tormented by two simple riddles:

The class is somewhat "known to the container",

I cannot imagine people for whom this info from container is useful: "I know about this class, but cannot say if you can get it"

as that would potentially be the full recursive autowiring required, which might be too much of a performance impact

if ->has(ClassName::class) affects performance, why ->has('ClassName') does not affects performance and wherein provides honest reliable answer ensuring that object can be retrieved.

@SvenRtbg
Copy link
Contributor

I'll try to make my suggestion more explicit: Create a class TopAccountProvider that basically has the same three methods: set, get and maybe has. set sets an instance of TopAccount however you are able to create it, and maybe this is even code that belongs into the TopAccountProvider itself. get returns the instance if it is already created, or may create a new instance if it knows how - otherwise would return null instead. has would return true or false depending on whether the existing instance is defined or still null.

I understand you're puzzled, and question the usefulness of has. You are probably right: Nobody is calling has often (don't want to claim "never", but close to "never" would be my expectation). The reason it is defined - as I believe it - is there is the feature of "container-in-container" that is part of the interface definition, but has no explicit method attached. A container can have an inner container that might be able to answer specific requests. And then there are implementations like https://packagist.org/packages/acclimate/container that intend to wrap multiple containers and query them one by one until a container can provide the requested entry. Using has is quite the usecase there. All in all I believe the current usage pattern is that nobody in their own business code is supposed to query a container at all because the object tree is already created by whatever request handling framework is in place.

So the only concern for the developer is to provide a working configuration to the container that is able to create any objects that have a lifetime of the incoming request itself, i.e. objects knowing how to process the request (controllers), how to validate parameters, how to query a backend database or other services. Explicitly not part of that is objects that represent some or all values of a specific request itself, that might be only created on the fly once processing of the request itself has already started.

And that means the configuration of the DI container is considered "fixed": It does not change at runtime, so the only useful method that MUST always deliver working objects is get(), and there barely is any need for calling has(), because once an object is requested, there is an actual need for it, it MUST exist, there is no alternative way to process the request at that point. So basically the only use case for has() would be to have some kind of nice error case handling - but calling get() and catching the NotFoundException would be basically the same result.

Your second question: There is no difference for these two cases. You are always requesting something identified by a string from the container. Any Classname::class constant is still just a string, but removes the necessity to assign another string to identify the class. And it includes the namespace automatically. Using "Classname" as a string is just the same, but you'd have to manually include a namespace if the configuration decision for the container was made to be classname-based.

The checks required for both cases are the same. No difference in performance when it comes to identifying whether or not PHP-DI can instantiate this class.

Just imagine two scenarios: a) rely on autowiring and b) have an explicit entry with some \DI\factory() definition. How does PHP-DI know it can instantiate these classes? It doesn't unless it actually tries to do so.

The autowired class might need classes that cannot be instantiated by themselves, or it requires some scalar parameters that do not provide a hint what to insert by themselves. And even if the scalar parameter for this class would be declared, any class it depends on might suffer the same problem, so without actively recursing the entire class tree it is impossible to really know for sure.

The explicitly defined class might also fail. It is impossible to know without executing the factory code itself, and this may try to pull more dependencies from the container that might fail, be it scalars or classes. It is basically the same situation with more explicit code.

PHP-DI cannot know for sure if it is eventually able to instantiate something. Having successfully been able in the past (i.e. having an entry in the resolvedEntries array makes it quite easy to return true immediately for has() and will return exactly the instance store when calling get() - but that's only a limited subset of what is possible to instantiate. If an instance has not yet been created, and doing it fully recursively is too much wasted performance just for has(), the only remaining option is to vaguely try to find hints if it is unmistakeably impossible (i.e. there is no such class in the codebase, and no explicit definition configured) - if the class exists or an entry is configured, PHP-DI will answer true (rough summary of my understanding of the executed code).

Note that the same problem might exist for any other container implementation that is relying on explicit configuration for every entry: It still might fail trying to instantiate something just because of a bug in the code that is trying to do it. Knowing a definition exists for something does not prove the defintion is able to provide whatever is expected. So in your case, it would actually solve the problem because no configured entry means you'll create one yourself, but in general the assumption that has() === true means the container will provide something when calling get() is wrong.

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

2 participants