Add support for defining factory parameters#428
Add support for defining factory parameters#428predakanga wants to merge 12 commits intoPHP-DI:5.3from
Conversation
|
Hi! That looks really good! I have a few comments, I'll put them in the diff directly. |
| * | ||
| * @return FactoryDefinitionHelper | ||
| */ | ||
| public function factoryParameter($parameter, $value) |
There was a problem hiding this comment.
I think factory(MyFactory::class)->parameter('foo', 'bar') would be simpler than factory(MyFactory::class)->factoryParameter('foo', 'bar').
There was a problem hiding this comment.
Happy to change it - I initially wrote it as ->parameter, changed it to ->factoryParameter to match methodParameter and constructorParameter
|
This is a really great pull request! Could you add a few functional test cases (here)? |
|
Oops almost forgot: could you update the documentation here too: https://github.com/PHP-DI/PHP-DI/blob/master/doc/php-definitions.md#factories |
|
Hey, thanks for the feedback! I'll update the PR shortly, though I'm not sure if rebasing will need me to create a new one - I selected to merge into PHP-DI:5.3 instead of master. |
|
Is there a reason you wanted to merge that into 5.3? To me it's a big feature, it deserves a minor version so it should go in master so that we can release it as 5.4. I'm afraid you'll have to close this one and reopen a new one though, I don't think you can edit the target branch? I had a look at the latest commits and the docs + new tests are awesome! |
|
There's no real reason to merge it into 5.3, I just made the initial fork from there. I'm rebasing it now, I'll open a new PR with that. |
2 participants
Fixes #362, matches your method name conventions, etc wherever possible