[11.x] Fix normalization in EventFake::assertListening #54951
+25
−20
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.
Taylor, I hope that you will consider this PR given that you have already rejected a similar (but faulty) PR #54907 submitted by @pandiselvamm based on him plagiarising my own hard work in #54878 (but without having retested it and corrected it).
The reasons that I think this PR should be considered are as follows:
The existing code is clearly intended to normalise expected and actual listener definitions so that
Object::class . "@method"
and[Object::class, 'method']
are equivalent. Unfortunately the current code only does this normalisation if the actual listener is@method
and NOT if only the expected listener is@method
. This PR makes this normalisation symmetrical.The code (excluding blank lines) is actually cleaner and slightly shorter than existing - this PR does not expand the code base.
It is a very minor point since the code is test code that is never run in production, but this PR moves some code out of a loop and improves (in a tiny way) the performance.
You rejected Fix: EventFake does not match Event:listen #54907 on the basis that "We never document this syntax." however as recently as v7.x this was in fact a documented syntax. I appreciate that this syntax is deprecated, and that existing tests will have worked around this, however I do foresee a possibility where package A is asserting that a listener
Object::class . "@method"
exists because this is what was used in package B, but then package B switches its definition to[Object::class, 'method']
and although the actuallisten
still works, the tests fail.The code in this PR is not identical to that in Fix: EventFake does not match Event:listen #54907 or EventFake does not match Event:listen documentation - both need fixing #54878. The scope of the code has been scaled back so that it is confined to fixing the specific issue without extending the functionality to supporting wildcard asserts, a bug relating expected events being closures has been fixed and the additional tests provided are more meaningful than in Fix: EventFake does not match Event:listen #54907, and the tests now run successfully.
Obviously I was somewhat annoyed that this other user stole my hard work and submitted it as his own, and then more annoyed that he failed to either submit a working PR or explain it properly and that consequently you rejected his PR. I sincerely hope that you will reconsider this PR on its own merits and not rejected it out of hand (in order to redress the harm that @pandiselvamm inflicted).
(Obviously) if you decide to accept this PR, then it should be forward ported to the 12.x branch too.
Many thanks. S