Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Draft
wants to merge 4 commits into
base: 11.x
Choose a base branch
from

Conversation

Sophist-UK
Copy link

@Sophist-UK Sophist-UK commented Mar 9, 2025

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

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

@taylorotwell taylorotwell marked this pull request as draft March 10, 2025 17:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant