Skip to content

[Proposal] Make dependency injection work with default values #80

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

Closed
wants to merge 4 commits into from
Closed

[Proposal] Make dependency injection work with default values #80

wants to merge 4 commits into from

Conversation

robclancy
Copy link
Contributor

I thought this would work by default, was using a component via dependency injection that has $configuration = null in the constructor and it threw the binding resolution exception. So I had a look and it was easy to make default values work, is there any reason to not do this?

$container = new Container;
$container->bind('IContainerContractStub', 'ContainerImplementationStub');
$class = $container->make('ContainerNestedDependentWithDefaultValueStub');
$this->assertTrue($class->inner instanceof ContainerDependentStub);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about assertInstanceOf()?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was just copying Taylors tests

@franzliedke
Copy link
Contributor

Nice one. Looks good.

$this->inner = $inner;
$this->innerArray = $innerArray;
$this->innerString = 'string';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is supposed to copy over the parameter?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, oops. Half asleep here haha

@jasonlewis
Copy link
Contributor

Seems logical to me. 👍

@robclancy
Copy link
Contributor Author

Are these being checked?

@HellPat
Copy link
Contributor

HellPat commented Jan 22, 2013

Worked for me 👍

@taylorotwell
Copy link
Member

Done. Implemented slightly differently.

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.

6 participants