framework icon indicating copy to clipboard operation
framework copied to clipboard

[11.x] Fix normalization in EventFake::assertListening

Open Sophist-UK opened this issue 9 months ago • 1 comments

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:

  1. 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.

  2. The code (excluding blank lines) is actually cleaner and slightly shorter than existing - this PR does not expand the code base.

  3. 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.

  4. You rejected #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 actual listen still works, the tests fail.

  5. The code in this PR is not identical to that in #54907 or #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 to expected events being closures has been fixed, and finally the additional tests provided are more meaningful than in #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

Sophist-UK avatar Mar 09 '25 10:03 Sophist-UK

@taylorotwell Is there a reason this has been marked as draft? Is there anything I need to do to make it not draft?

Sophist-UK avatar Mar 16 '25 16:03 Sophist-UK