Fixed parameters resolution of generated factories #17
Conversation
enumag
force-pushed
the
factories-fix
branch
from
30ef8c6
to
4c669c7
Compare
|
I've modified the tests, didn't have to add new ones in the end. I've also prepared another branch where the fix is separated to four commits - failing test, fix, failing test, better fix - to show why the fix needs to be like this. The PR itself contains those four commits squashed. |
enumag
force-pushed
the
factories-fix
branch
from
4c669c7
to
4750c2b
Compare
| @@ -306,6 +306,9 @@ private function resolveImplement(ServiceDefinition $def, $name) | |||
|
|
|||
| if (!$def->parameters) { | |||
| $ctorParams = array(); | |||
| if ((!$def->factory || !$def->factory->entity && !$def->factory->arguments) && $def->class) { | |||
There was a problem hiding this comment.
Ambiguous && and || resolution.
There was a problem hiding this comment.
You mean operators priority? Should I add (...)?
There was a problem hiding this comment.
By the way the && $def->class can be removed and tests will still pass.
|
@enumag Why did you modified existing test and didn't add new one? |
|
Because I thought it would be redundant. But ok, I'll add new tests instead. |
|
@dg Also what about && and ||? Should I add brackets for readability or not? I didn't find anything about it in Nette coding style so I did it without brackets whis was possible according to PHP operator precedence. |
|
Whether it is 100% redundant, it is ok. |
enumag
force-pushed
the
factories-fix
branch
from
4750c2b
to
e76dc64
Compare
|
Well it might not be 100% so I added them. |
It is really less understandable, || can be removed this way |
|
@dg Nope, that failed with E_NOTICE Trying to get property of non-object. By the way && has greater priority than ||. The code I used is (intentionaly) equivalent to this: |
|
Now the parameters resolution magic works even if the class gets some arguments from config. |
enumag
force-pushed
the
factories-fix
branch
from
d30c6d0
to
e4dfa46
Compare
| if (!$def->factory || !$def->factory->entity) { | ||
| $def->setFactory($def->class, $def->factory ? $def->factory->arguments : array()); | ||
| } | ||
| if ($def->factory && ($class = $this->resolveEntityClass($def->factory, array($name => 1))) |
There was a problem hiding this comment.
@dg What is the array($name => 1) here for? You added it when merging the feature but removing it doesn't break any test.
There was a problem hiding this comment.
resolveEntityClass() uses it when throws exception.
dg
force-pushed
the
master
branch
6 times, most recently
from
d349d3f
to
7da1abe
Compare
|
ping @dg, If there is any problem with this PR I'll be happy to fix it. |
The previous commit doesn't seem to work as expected in all cases (http://forum.nette.org/cs/20601-generovane-tovarnicky-s-parametry), most probably because it was rebased several times. Of course I will add more tests for this.