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

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

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

unkind
Copy link
Contributor

@unkind 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
Copy link
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
Copy link
Contributor Author

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
Copy link
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
Copy link
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 feature-array-as-valid-parameters-values branch September 5, 2013 16:10
@mnapoli
Copy link
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
Copy link
Contributor Author

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
Copy link
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
Copy link
Contributor Author

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
Copy link
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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants