Expose detection of closure comparisons - #163
Closed
sebastianbergmann wants to merge 1 commit into
Closed
Conversation
API Surface ChangesIf any of the additions below are not intended as public API, mark them with New API SurfaceMethods
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #163 +/- ##
============================================
+ Coverage 94.96% 95.02% +0.05%
- Complexity 229 232 +3
============================================
Files 18 18
Lines 616 623 +7
============================================
+ Hits 585 592 +7
Misses 31 31 ☔ View full report in Codecov by Sentry. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ClosureComparatordecides closure equality with a heuristic: two closures are "equal" when they share the same declaration site, the same bound$this, and equal capturedusevariables. That can quietly return equal for closures that are not meaningfully interchangeable.PHPUnit wants to surface this and raise a (possibly opt-out) warning when
assertEquals()(and the other comparator-based assertions) end up comparing closures so users learn that they rely on fragile closure comparison rather than getting a silent pass.Equality assertions run constantly, often over large arrays and object graphs. PHPUnit must not pay a second full traversal of the operands just to ask "was a closure involved?" as that tax would be paid on every assertion, including the overwhelming majority that contain no closures at all.
The comparison already walks the entire graph:
ArrayComparator/ObjectComparatordispatch every nested node throughFactory::getComparatorFor(), which routes closure-vs-closure nodes toClosureComparator. So the cheap place to detect closure comparison is inside the comparison that's already happening. We can letClosureComparatorrecord the fact, and let the consumer read it afterward. No second traversal, effectively free.Factorygains a per-instance boolean flag and a small API (all additive,Factoryis under the backward compatibility promise, so this is BC-safe):recordClosureComparison()sets the flag. Marked@internal; it's the write side, called only byClosureComparatorclosureComparisonOccurred()is the consumer-facing queryresetClosureComparisonTracking()clears the flag so a consumer can scope it to a single comparisonClosureComparator::assertEquals()calls$this->factory()->recordClosureComparison()as its first statement. Because every default comparator is registered into the sameFactoryinstance viasetFactory(), the flag set deep inside a nested comparison is visible to theFactorythe consumer holds — even when the closures are buried inside arrays or object properties.The flag is set to
trueonly when a closure is compared against a closure. This is exactly the silent, heuristic-driven case worth warning about. The flag is not set totruefor one-sided cases (a closure compared against a scalar /null, or under a key that exists on only one side), because those already produce a visible assertion failure. So the cheap signal targets the genuinely silent risk, not the cases that already fail loudly.