Fix matching for '@array@||@null@' #493
Conversation
| new Matcher\ChainMatcher( | ||
| 'array', | ||
| $backtrace, | ||
| [ | ||
| $orMatcher, | ||
| new Matcher\OrMatcher($backtrace, $orMatchers = clone $scalarMatchers), |
There was a problem hiding this comment.
have you tried it without a clone?
There was a problem hiding this comment.
Deleting the clone works but I added it here for a specific reason. Since the factory describes the following matcher priorities:
// Matchers are registered in order of matching
// 1) all scalars
// 2) json/xml
// 3) array
// 4) or "||"
// 5) full text
I have decided to clone the scalair matchers in order to keep theses priorities on the main matcher.
There was a problem hiding this comment.
and without clone that order is changed?
|
in order to fix that static analysis error run |
There was a problem hiding this comment.
So here are two things that need to be clarified/added before this can be merged:
- I would love to understand why that cloning is necessary and what are the consequences of not using clone
- There needs to be some tests added that would fail before this fix is implemented
|
@norberttech I have added a test covering this fix. For the cloning part, I keep BC strictly according to the documented matcher priorites, if you prefer, I can drop it but the, I would also drop the registering of the original array matcher on the main matcher chain since it would become part of the scalar matcher chain. By the way, if I don't clone it, the test becomes broken. |
This is what I wanted to know, thanks! |
Change Log
Added
Fixed
@null@||@array@(again) #471Changed
Removed
Deprecated
Security
Description
This PR fixes #471 by making aware the "OrMatcher" about the "ArrayMatcher".