Allow indexed non-empty arrays being valid parameter values #106

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
2 participants
@unkind
Contributor

unkind commented Sep 2, 2013

Currently, there is no way to store indexed arrays as values except direct call $container->set(...):

DI\Definition\Exception\DefinitionException: Invalid key '0' in definition of entry 'foo'; Valid keys are: class, scope, lazy, constructor, properties, methods

By the way, it's impossible to discern type of parameter (simple value or service) in case of empty array.

P.S. I didn't add this feature for XmlDefinitionFileLoader, anyway it looks not fully supported at this moment.

@mnapoli

This comment has been minimized.

Show comment
Hide comment
@mnapoli

mnapoli Sep 4, 2013

Member

Thanks for that work, still I'm conflicted: indeed, that's a problem that we can't define arrays for now. But this solution is only partial: it works only for non-indexed and non-empty arrays.

So there's confusion with that solution, because users might think that it's possible to define arrays, whereas it's very restricted.

Anyway that's a fundamental problem, your solution could be merged for a 3.x version. For 4.0, I'd like to fix this definitely (don't know yet what's the best solution for that).

Member

mnapoli commented Sep 4, 2013

Thanks for that work, still I'm conflicted: indeed, that's a problem that we can't define arrays for now. But this solution is only partial: it works only for non-indexed and non-empty arrays.

So there's confusion with that solution, because users might think that it's possible to define arrays, whereas it's very restricted.

Anyway that's a fundamental problem, your solution could be merged for a 3.x version. For 4.0, I'd like to fix this definitely (don't know yet what's the best solution for that).

@unkind

This comment has been minimized.

Show comment
Hide comment
@unkind

unkind Sep 4, 2013

Contributor

I prefer this one to be honest:

parameters:
    value1: 42
services:
    service1: ~

In 3.x it's possible to create method like $builder->addValueDefinitionsFromFile().

Container without arrays is very restricted for me. For instance, I cannot define list of emails for reports. Current format doesn't allow me to call some method twice. So I cannot define dependency neither

$reportCommand->addEmail('bob@acme.example.com');
$reportCommand->addEmail('alice@acme.example.com');

nor

$reportCommand->setEmailList(['bob@acme.example.com', 'alice@acme.example.com']);
Contributor

unkind commented Sep 4, 2013

I prefer this one to be honest:

parameters:
    value1: 42
services:
    service1: ~

In 3.x it's possible to create method like $builder->addValueDefinitionsFromFile().

Container without arrays is very restricted for me. For instance, I cannot define list of emails for reports. Current format doesn't allow me to call some method twice. So I cannot define dependency neither

$reportCommand->addEmail('bob@acme.example.com');
$reportCommand->addEmail('alice@acme.example.com');

nor

$reportCommand->setEmailList(['bob@acme.example.com', 'alice@acme.example.com']);
@mnapoli

This comment has been minimized.

Show comment
Hide comment
@mnapoli

mnapoli Sep 4, 2013

Member

I prefer this one to be honest:

parameters:
    value1: 42
services:
    service1: ~

That's what I was thinking for 4.0. It's less "simple", but at least it will allow to offer 100% functionalities.

it works only for non-indexed and non-empty arrays

My point is you probably don't need assoc arrays since you can write

value1.key1: 42
value1.key2: "abc"

+1, totally right

but there is no way to create lists at all: current format doesn't allow me to call some method twice. So I cannot > define dependency neither

$reportCommand->addEmail('bob@acme.example.com');
$reportCommand->addEmail('alice@acme.example.com');

nor

$reportCommand->setEmailList(['bob@acme.example.com', 'alice@acme.example.com']);

Yes I'm facing the same limitations, that's why I want to move 4.0 forward.

So to sum up, OK to merge this feature in 3.4 while waiting for a better solution for 4.0.

I'll do it later I can't right now.

Thanks

Member

mnapoli commented Sep 4, 2013

I prefer this one to be honest:

parameters:
    value1: 42
services:
    service1: ~

That's what I was thinking for 4.0. It's less "simple", but at least it will allow to offer 100% functionalities.

it works only for non-indexed and non-empty arrays

My point is you probably don't need assoc arrays since you can write

value1.key1: 42
value1.key2: "abc"

+1, totally right

but there is no way to create lists at all: current format doesn't allow me to call some method twice. So I cannot > define dependency neither

$reportCommand->addEmail('bob@acme.example.com');
$reportCommand->addEmail('alice@acme.example.com');

nor

$reportCommand->setEmailList(['bob@acme.example.com', 'alice@acme.example.com']);

Yes I'm facing the same limitations, that's why I want to move 4.0 forward.

So to sum up, OK to merge this feature in 3.4 while waiting for a better solution for 4.0.

I'll do it later I can't right now.

Thanks

mnapoli added a commit that referenced this pull request Sep 5, 2013

@mnapoli

This comment has been minimized.

Show comment
Hide comment
@mnapoli

mnapoli Sep 5, 2013

Member

This is now merged in the 3.4 branch (manually merged), thank you for updating the docs too.

Member

mnapoli commented Sep 5, 2013

This is now merged in the 3.4 branch (manually merged), thank you for updating the docs too.

@mnapoli mnapoli closed this Sep 5, 2013

@unkind unkind deleted the unkind:feature-array-as-valid-parameters-values branch Sep 5, 2013

@mnapoli

This comment has been minimized.

Show comment
Hide comment
@mnapoli

mnapoli Sep 24, 2013

Member

FYI v3.4 is now published (http://mnapoli.fr/PHP-DI/news/04-php-di-3-4), thanks again

Member

mnapoli commented Sep 24, 2013

FYI v3.4 is now published (http://mnapoli.fr/PHP-DI/news/04-php-di-3-4), thanks again

@unkind

This comment has been minimized.

Show comment
Hide comment
@unkind

unkind Sep 24, 2013

Contributor

There is a typo: "Note that the arrays have to be non-indexed", you wanted to say "indexed non-empty".

Contributor

unkind commented Sep 24, 2013

There is a typo: "Note that the arrays have to be non-indexed", you wanted to say "indexed non-empty".

@mnapoli

This comment has been minimized.

Show comment
Hide comment
@mnapoli

mnapoli Sep 24, 2013

Member

Right I forgot about non-empty. However they need to be "not (string) indexed" too, i.e.:

foo:
  bar: baz
  bim: bam

is not possible, whereas this works:

foo:
  - baz
  - bam

Maybe "non-indexed" is not really appropriate, do you see another way of putting it?

Member

mnapoli commented Sep 24, 2013

Right I forgot about non-empty. However they need to be "not (string) indexed" too, i.e.:

foo:
  bar: baz
  bim: bam

is not possible, whereas this works:

foo:
  - baz
  - bam

Maybe "non-indexed" is not really appropriate, do you see another way of putting it?

@unkind

This comment has been minimized.

Show comment
Hide comment
@unkind

unkind Sep 24, 2013

Contributor

Well, "indexed" means numeric keys for me, "string indexed" sounds weird. You can replace it with "list of values", for instance.

Contributor

unkind commented Sep 24, 2013

Well, "indexed" means numeric keys for me, "string indexed" sounds weird. You can replace it with "list of values", for instance.

@mnapoli

This comment has been minimized.

Show comment
Hide comment
@mnapoli

mnapoli Sep 25, 2013

Member

Thanks I read the official doc and the proper terms are indexed VS associative array.

So I edited the sentence to:

Note that the arrays can't be associative arrays, and must not be empty.

Member

mnapoli commented Sep 25, 2013

Thanks I read the official doc and the proper terms are indexed VS associative array.

So I edited the sentence to:

Note that the arrays can't be associative arrays, and must not be empty.

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