fix Autowiring tests of #18144 #18436
Conversation
| Q | A |
|---|---|
| Branch? | master |
| Bug fix? | yes |
| Tests pass? | yes |
| Fixed issue | #18422 (comment) |
| License | MIT |
|
Wow! It's all green again! ping @nicolas-grekas :) |
|
|
||
| $newSelf = AutowirePass::createResourceForClass($reflectionClass); | ||
|
|
||
| return $this == $newSelf; |
There was a problem hiding this comment.
one-liner + strict comparison
There was a problem hiding this comment.
I thought about inlining but this was the original style (ref 49271e8#diff-04e2904117929f6ffe784d5be3cb1103L49).
So I'm fine either way. Final call ?
Doesn't a strict comparison always return true ? We need to compare their data, right ?
There was a problem hiding this comment.
In fact triple equals wouldn't fit as objects have different identities. Yet this is strange: @weaverryan shouldn't we touch($this->filePath) here when the objects are equals?
Would it be better to compare properties using strict comparisons? This could be done with:
((array) $this === (array) AutowirePass::createResourceForClass($reflectionClass)
There was a problem hiding this comment.
You're right, it's always false, hence the tests would pass anyway. I don't think we need touch since my guess is that it handles the case where the original class file does not exist anymore but the same class can be autoloaded from elsewhere (? weird or not I don't know :).
In any case we should at least add comments here about what's happening and at best add a test to cover regressions like the two previous PR's.
?
There was a problem hiding this comment.
@nicolas-grekas I'm sorry, what would be the point of the touch() - I'm not sure I understand?
And yes, would could definitely cast into arrays to compare the properties only - that's definitely "in the spirit" of what we want to compare. Really, we could just compare the autowiringMetadata paths (as we've already checked things on filePath). But I didn't want to make this public just for this.
So:
- +1 for casting both objects into arrays
- I don't understand the
touch($this->filepath)- would that just be to update the file so that it looks fresh automatically next time and avoids extra checking? If so, we could do that for sure - but probably the rest of the logic is very fast anyways - I don't understand the comment
You're right, it's always false, hence the tests would pass anyway.
Sorry for being slow on this :) - thanks to both of you for the help!
There was a problem hiding this comment.
Now fixed.
@weaverryan originally the PR was about fixing a test where this function should return false.
In PHP, when comparing object using strict === it returns true if this is the same instance of the class, in that case it would always be false, so the test would always pass.
We could use == to compare the objects since it returns true for different instances with identical properties, this is the same as casting them to arrays being strict, without checking the class.
|
Status: needs work |
1e94811 to
0cbf04a
Compare
|
Status: Needs Review |
|
👍 Status: Reviewed |
|
Thank you @HeahDude. |
This PR was merged into the 3.1-dev branch. Discussion ---------- fix Autowiring tests of #18144 | Q | A | ------------- | --- | Branch? | master | Bug fix? | yes | Tests pass? | yes | Fixed issue | #18422 (comment) | License | MIT Commits ------- 0cbf04a [DI] fix Autowiring tests of #18144