Support for non-object concrete values

DvdGiessen commented 3 years ago

After upgrading from 3.x to the latest 4.x, I noticed that non-object values are no longer working correctly due to the newly added typehints.

Since this wasn't mentioned in the changelog / breaking changes, opening an issue so we can either resolve this and make Container work again for non-objects, or to document that Container only supports objects as values.

Note that PSR-11 specifies the returned value as mixed, and also in other places in this library there appears to be attention given to support types other than objects, for example in Definition.php:174-180.

Minimal example to test this breaking change:

<?php
declare(strict_types=1);
require_once __DIR__ . '/vendor/autoload.php';

$container = new \League\Container\Container();

$container->add('my-array', [1, 2, 3]);
$container->add('my-number', 123);
$container->add('my-string', 'foo bar');
$container->add('my-generated-array', [\DateTimeZone::class, 'listIdentifiers']);
$container->add('my-generated-number', 'time');
$container->add('my-generated-string', function (): string {
    return bin2hex(random_bytes(8));
});

// Each line below works with 3.3.5 and breaks with 4.1.1
$container->get('my-array');
$container->get('my-number');
$container->get('my-string');
$container->get('my-generated-array');
$container->get('my-generated-number');
$container->get('my-generated-string');

In 4.x, they all throw a TypeError: InflectorAggregate::inflect(): Argument #1 ($object) must be of type object, array given.

Example is of course completely artificial and doesn't represent actual use cases.

Concrete use cases for non-objects come in many forms and shapes. Reasonable ones might be generated values, resource type values (while becoming more and more rare, they do still exist). My own use case which triggered this is I am sharing an array of objects (which, after looking into this issue, I learned also exists the "tagging" functionality for, which is pretty cool actually. But I would argue it would still be nice for the naive approach of sharing an array of objects to work).

So, question is, does Container aim to support values of all types, or only objects?

If the former, we should start by adding a bunch of unit tests for various non-object concrete values, and then fix all the resulting problems (I expect there to be more than just these type errors. One design challenge will be distinguishing between what's intended as a callable or a value while keeping the/a very simple API. As seen in the example: can you have the string "time" as a value).
If the latter, this should be documented somewhere clearly, so that this limitation doesn't come as a surprise to users.

philipobenito commented 3 years ago

This definitely seems like a bug, thanks for creating this, I’ll look at it in the next few days.

Sent from ProtonMail for iOS

On Fri, Jul 23, 2021 at 18:23, Daniël van de Giessen @.***> wrote:

After upgrading from 3.x to the latest 4.x, I noticed that non-object values are no longer working correctly due to the newly added typehints.

Since this wasn't mentioned in the changelog / breaking changes, opening an issue so we can either resolve this and make Container work again for non-objects, or to document that Container only supports objects as values.

Note that PSR-11 specifies the returned value as mixed, and also in other places in this library there appears to be attention given to support types other than objects, for example in Definition.php:174-180.

Minimal example to test this breaking change:

<?php

declare

(strict_types=

1

);

require_once

DIR .

'/vendor/autoload.php'

;

$

container

=

new

\

League

\

Container

\

Container

();

$

container

->

add

(

'my-array'

, [

1

,

2

,

3

]);

$

container

->

add

(

'my-number'

,

123

);

$

container

->

add

(

'my-string'

,

'foo bar'

);

$

container

->

add

(

'my-generated-array'

, [\

DateTimeZone

::class,

'listIdentifiers'

]);

$

container

->

add

(

'my-generated-number'

,

'time'

);

$

container

->

add

(

'my-generated-string'

,

function

():

string

{

return

bin2hex

(

random_bytes

(

8

)); });

// Each line below works with 3.3.5 and breaks with 4.1.1

$

container

->

get

(

'my-array'

);

$

container

->

get

(

'my-number'

);

$

container

->

get

(

'my-string'

);

$

container

->

get

(

'my-generated-array'

);

$

container

->

get

(

'my-generated-number'

);

$

container

->

get

(

'my-generated-string'

);

In 4.x, they all throw a TypeError: InflectorAggregate::inflect(): Argument #1 ($object) must be of type object, array given.

Concrete use cases for non-objects come in many forms and shapes. Reasonable ones might be generated values, resource type values (while becoming more and more rare, they do still exist). My own use case which triggered this is I am sharing an array of objects (which, after looking into this issue, I learned also exists the "tagging" functionality for, which is pretty cool actually. But I would argue it would still be nice for the naive approach of sharing an array of objects to work).

So, question is, does Container aim to support values of all types, or only objects?

If the former, we should start by adding a bunch of unit tests for various non-object concrete values, and then fix all the resulting problems (I expect there to be more than just these type errors).

If the latter, this should be documented somewhere clearly, so that this limitation doesn't come as a surprise to users.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or unsubscribe.

philipobenito commented 3 years ago

Failing test here https://github.com/thephpleague/container/commit/2c3abde27d8f8f8005fd76473bf0268368bfedfa

Sorting fix now

philipobenito commented 3 years ago

Fixed in release 4.1.2

https://github.com/thephpleague/container/releases/tag/4.1.2

thephpleague / container

Support for non-object concrete values #231