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

DI doesn't resolve, if an optional parameter comes before other definitions #520

Closed
exts opened this issue Aug 3, 2017 · 4 comments
Closed
Labels
Milestone

Comments

@exts
Copy link

exts commented Aug 3, 2017

For example there's some legacy code in a project I'm working on:

Object (
    class = LanguageWebService
    scope = singleton
    lazy = false
    __construct(
        $language_directory = #UNDEFINED#
        $request = get(Symfony\Component\HttpFoundation\Request)
        $permissions = get(Permissions)
    )
)

Even if the $language_directory is set to null by default (optional) php-di doesn't properly pass a optional null value when trying to resolve the dependency. If $language_directory is the last parameter it works as normal.

@juliangut
Copy link
Contributor

As a hint optional parameters should always be placed at the end of parameter list

@exts
Copy link
Author

exts commented Aug 3, 2017

@juliangut right, but I feel like it's still easy to detect that using ReflectionParameter::getDefaultValue and ::isOptional

@mnapoli
Copy link
Member

mnapoli commented Aug 5, 2017

Thanks this is indeed a bug (though not best practice for code as @juliangut mentioned), I was suprised because it is supposed to be covered by tests but actually there is some weird behavior of PHP's reflection.

I've pushed #521 to fix it. It will be fixed in 6.0 though.

@exts
Copy link
Author

exts commented Aug 5, 2017

alright cool, yeah when working with a lot of legacy code & having to change parameter order all at once would slow me down. So this fix really helps me out until I can solve those issues. Thanks a lot for fixing this :)

mnapoli added a commit that referenced this issue Aug 6, 2017
Fix #520: Error if an optional parameter is before required parameters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants