Fix include orders #314
Fix include orders #314
Conversation
grobx
commented
Feb 23, 2016
grobx
commented
- close Suggestion on MergeInclude #164
| } | ||
| } | ||
|
|
||
| return $data; | ||
| return $includeData; |
There was a problem hiding this comment.
@piribes I feel like changing $includeData makes the intention here less clear, since conceptually $includeData should be appended to $data. Instead, let's just create a new variable, $newData or something, and return it. Does that make sense?
There was a problem hiding this comment.
It makes sense to me, i have updated the code.
There was a problem hiding this comment.
@theofidry i created $newData instead of reusing $includeData just for code clarity, as stated here by @tshelburne
If you think it is incorrect, there's no problem, we can rollback whenever we want, of course :)
There was a problem hiding this comment.
To me it's just that in this case if the name is not clear enough it should be changed in the signature :)
That being said I'm saying that because I find this line useless that's it. If you or @tshelburne find it more clear to do it this way I'm fine with it.
* close #164
| foreach ($includeData as $class => $fixtures) { | ||
| if (isset($data[$class])) { | ||
| $data[$class] = array_merge($fixtures, $data[$class]); | ||
| $newData = $includeData; |
There was a problem hiding this comment.
$newData is unneeded, you can use $includeData right away
There was a problem hiding this comment.
This is per my request earlier - I feel like this variable name makes the intention much more clear.
|
Any chance ? |
|
👍 As far as I can tell there is no BC break so can be released in a patch :) |
|
Makes sense to me - @piribes thanks! |
|
my pleasure |
