-
Notifications
You must be signed in to change notification settings - Fork 11.3k
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
EventFake does not match Event:listen documentation - both need fixing #54878
Comments
The documentation was moved, not deprecated: https://laravel.com/docs/12.x/events#testing
Can you submit failing tests to describe the issue to this repository? |
composer create-project laravel/laravel:9.x EventFakerTest
php artisan make:event TestEvent
php artisan make:listener TestListener --event=TestEvent
rm tests/Feature/ExampleTest.php tests/Unit/ExampleTest.php
php artisan make:test EventFaker EventFakerTest.php <?php
namespace Tests\Unit;
use App\Events\TestEvent;
use App\Listeners\TestListener;
use Illuminate\Support\Facades\Event;
use Tests\TestCase;
class EventFakerTest extends TestCase
{
public function setup():void
{
parent::setup();
Event::fake();
}
/**
* The following are (or should be) functionally identical:
*
* Event::listen(TestEvent::class, TestListener::class);
* Event::listen(TestEvent::class, [TestListener::class]);
* Event::listen(TestEvent::class, [TestListener::class, 'handle']);
* Event::listen(TestEvent::class, TestListener::class . '@handle');
*
* because the same Event triggers the same method of the same Listener.
*
* Since you don't necessarily know which of the above forms the listener was attached with
* testing for one should match against the others.
*
**/
public function test_matched_assertListening_class_only()
{
Event::listen(TestEvent::class, TestListener::class);
Event::assertListening(TestEvent::class, TestListener::class);
}
public function test_matched_assertListening_class_list()
{
Event::listen(TestEvent::class, [TestListener::class]);
Event::assertListening(TestEvent::class, [TestListener::class]);
}
public function test_matched_assertListening_class_and_method()
{
Event::listen(TestEvent::class, [TestListener::class, 'handle']);
Event::assertListening(TestEvent::class, [TestListener::class, 'handle']);
}
public function test_matched_assertListening_class_at_method()
{
Event::listen(TestEvent::class, TestListener::class . '@handle');
Event::assertListening(TestEvent::class, TestListener::class . '@handle');
}
public function test_matched_assertListening_class_only_vs_class_list()
{
Event::listen(TestEvent::class, TestListener::class);
Event::assertListening(TestEvent::class, [TestListener::class]);
}
public function test_mismatched_assertListening_class_only_vs_class_and_method()
{
Event::listen(TestEvent::class, TestListener::class);
Event::assertListening(TestEvent::class, [TestListener::class, 'handle']);
}
public function test_mismatched_assertListening_class_only_vs_class_at_method()
{
Event::listen(TestEvent::class, TestListener::class);
Event::assertListening(TestEvent::class, TestListener::class . '@handle');
}
public function test_matched_assertListening_class_list_vs_class_only()
{
Event::listen(TestEvent::class, [TestListener::class]);
Event::assertListening(TestEvent::class, TestListener::class);
}
public function test_mismatched_assertListening_class_list_vs_class_and_method()
{
Event::listen(TestEvent::class, [TestListener::class]);
Event::assertListening(TestEvent::class, [TestListener::class, 'handle']);
}
public function test_mismatched_assertListening_class_list_vs_class_at_method()
{
Event::listen(TestEvent::class, [TestListener::class]);
Event::assertListening(TestEvent::class, TestListener::class . '@handle');
}
public function test_mismatched_assertListening_class_and_method_vs_class_only()
{
Event::listen(TestEvent::class, [TestListener::class, 'handle']);
Event::assertListening(TestEvent::class, TestListener::class);
}
public function test_matched_assertListening_class_and_method_vs_class_list()
{
Event::listen(TestEvent::class, [TestListener::class, 'handle']);
Event::assertListening(TestEvent::class, [TestListener::class]);
}
public function test_mismatched_assertListening_class_and_method_vs_class_at_method()
{
Event::listen(TestEvent::class, [TestListener::class, 'handle']);
Event::assertListening(TestEvent::class, TestListener::class . '@handle');
}
public function test_mismatched_assertListening_class_at_method_vs_class_only()
{
Event::listen(TestEvent::class, TestListener::class . '@handle');
Event::assertListening(TestEvent::class, TestListener::class);
}
public function test_matched_assertListening_class_at_method_vs_class_list()
{
Event::listen(TestEvent::class, TestListener::class . '@handle');
Event::assertListening(TestEvent::class, [TestListener::class]);
}
public function test_mismatched_assertListening_class_at_method_vs_class_and_method()
{
Event::listen(TestEvent::class, TestListener::class . '@handle');
Event::assertListening(TestEvent::class, [TestListener::class, 'handle']);
}
} Results with L9.x:
|
I tried my version of the code and made some further changes and here is an improved version of the code (and a minor tweak to remove duplication): /**
* Assert if an event has a listener attached to it.
*
* @param string $expectedEvent
* @param string|array $expectedListener
* @return void
*/
public function assertListening($expectedEvent, $expectedListener)
{
$normalizedListener = $expectedListener;
if (is_array($normalizedListener) && count($normalizedListener) == 1 && is_string($normalizedListener[0])) {
$normalizedListener = $normalizedListener[0];
}
if (is_string($normalizedListener)) {
if (Str::contains($normalizedListener, '@')) {
$normalizedListener = Str::parseCallback($normalizedListener);
} else {
$normalizedListener = [$normalizedListener, 'handle'];
}
}
foreach ($this->dispatcher->getListeners($expectedEvent) as $listenerClosure) {
$actualListener = (new ReflectionFunction($listenerClosure))
->getStaticVariables()['listener'];
if (is_array($actualListener) && count($actualListener) == 1 && is_string($actualListener[0])) {
$actualListener = $actualListener[0];
}
if (is_string($actualListener)) {
if (Str::contains($actualListener, '@')) {
$actualListener = Str::parseCallback($actualListener);
} else {
$actualListener = [$actualListener, 'handle'];
}
}
if ($actualListener === $normalizedListener ||
($actualListener instanceof Closure &&
$normalizedListener === Closure::class)) {
PHPUnit::assertTrue(true);
return;
}
}
PHPUnit::assertTrue(
false,
sprintf(
'Event [%s] does not have the [%s] listener attached to it',
$expectedEvent,
print_r($expectedListener, true)
)
);
} Removing duplication: /**
* Assert if an event has a listener attached to it.
*
* @param string $expectedEvent
* @param string|array $expectedListener
* @return void
*/
public function assertListening($expectedEvent, $expectedListener)
{
$normalizedListener = $this->parseListener($expectedListener);
foreach ($this->dispatcher->getListeners($expectedEvent) as $listenerClosure) {
$actualListener = $this->parseListener((new ReflectionFunction($listenerClosure))
->getStaticVariables()['listener']);
if ($actualListener === $normalizedListener ||
($actualListener instanceof Closure &&
$normalizedListener === Closure::class)) {
PHPUnit::assertTrue(true);
return;
}
}
PHPUnit::assertTrue(
false,
sprintf(
'Event [%s] does not have the [%s] listener attached to it',
$expectedEvent,
print_r($expectedListener, true)
)
);
}
protected function parseListener($listener)
{
if (is_array($listener) && count($listener) == 1 && is_string($listener[0])) {
$listener = $listener[0];
}
if (is_string($listener)) {
if (Str::contains($listener, '@')) {
$listener = Str::parseCallback($listener);
} else {
$listener = [$listener, 'handle'];
}
}
return $listener;
} |
Additionally I would propose that we implement wildcard tests - so if you explicitly specify an expected listener method containing a "*" then any matching actual listener classes would make assertListening pass. <?php
namespace Tests\Unit;
use App\Events\TestEvent;
use App\Listeners\TestListener;
use Illuminate\Support\Facades\Event;
use Tests\TestCase;
class EventFakerTest extends TestCase
{
public function setup():void
{
parent::setup();
Event::fake();
}
/**
* The following are (or should be) functionally identical:
*
* Event::listen(TestEvent::class, TestListener::class);
* Event::listen(TestEvent::class, [TestListener::class]);
* Event::listen(TestEvent::class, [TestListener::class, 'handle']);
* Event::listen(TestEvent::class, TestListener::class . '@handle');
*
* because the same Event triggers the same method of the same Listener.
*
* Since you don't necessarily know which of the above forms the listener was attached with
* testing for one should match against the others.
*
* Additionally, if you explicitly specify the expected method containing a * then
* it should match any listeners that match on wildcards.
**/
public function test_matched_assertListening_class_only()
{
Event::listen(TestEvent::class, TestListener::class);
Event::assertListening(TestEvent::class, TestListener::class);
}
public function test_matched_assertListening_class_list()
{
Event::listen(TestEvent::class, [TestListener::class]);
Event::assertListening(TestEvent::class, [TestListener::class]);
}
public function test_matched_assertListening_class_and_method()
{
Event::listen(TestEvent::class, [TestListener::class, 'handle']);
Event::assertListening(TestEvent::class, [TestListener::class, 'handle']);
}
public function test_matched_assertListening_class_at_method()
{
Event::listen(TestEvent::class, TestListener::class . '@handle');
Event::assertListening(TestEvent::class, TestListener::class . '@handle');
}
public function test_matched_assertListening_class_only_vs_class_list()
{
Event::listen(TestEvent::class, TestListener::class);
Event::assertListening(TestEvent::class, [TestListener::class]);
}
public function test_mismatched_assertListening_class_only_vs_class_and_method()
{
Event::listen(TestEvent::class, TestListener::class);
Event::assertListening(TestEvent::class, [TestListener::class, 'handle']);
}
public function test_mismatched_assertListening_class_only_vs_class_at_method()
{
Event::listen(TestEvent::class, TestListener::class);
Event::assertListening(TestEvent::class, TestListener::class . '@handle');
}
public function test_matched_assertListening_class_list_vs_class_only()
{
Event::listen(TestEvent::class, [TestListener::class]);
Event::assertListening(TestEvent::class, TestListener::class);
}
public function test_mismatched_assertListening_class_list_vs_class_and_method()
{
Event::listen(TestEvent::class, [TestListener::class]);
Event::assertListening(TestEvent::class, [TestListener::class, 'handle']);
}
public function test_mismatched_assertListening_class_list_vs_class_at_method()
{
Event::listen(TestEvent::class, [TestListener::class]);
Event::assertListening(TestEvent::class, TestListener::class . '@handle');
}
public function test_mismatched_assertListening_class_and_method_vs_class_only()
{
Event::listen(TestEvent::class, [TestListener::class, 'handle']);
Event::assertListening(TestEvent::class, TestListener::class);
}
public function test_mismatched_assertListening_class_and_method_vs_class_list()
{
Event::listen(TestEvent::class, [TestListener::class, 'handle']);
Event::assertListening(TestEvent::class, [TestListener::class]);
}
public function test_mismatched_assertListening_class_and_method_vs_class_at_method()
{
Event::listen(TestEvent::class, [TestListener::class, 'handle']);
Event::assertListening(TestEvent::class, TestListener::class . '@handle');
}
public function test_mismatched_assertListening_class_at_method_vs_class_only()
{
Event::listen(TestEvent::class, TestListener::class . '@handle');
Event::assertListening(TestEvent::class, TestListener::class);
}
public function test_mismatched_assertListening_class_at_method_vs_class_list()
{
Event::listen(TestEvent::class, TestListener::class . '@handle');
Event::assertListening(TestEvent::class, [TestListener::class]);
}
public function test_mismatched_assertListening_class_at_method_vs_class_and_method()
{
Event::listen(TestEvent::class, TestListener::class . '@handle');
Event::assertListening(TestEvent::class, [TestListener::class, 'handle']);
}
public function test_mismatched_assertListening_class_list_vs_class_only()
{
Event::listen(TestEvent::class, [TestListener::class]);
Event::assertListening(TestEvent::class, TestListener::class);
}
public function test_wildcard_assertListening_class_only_vs_class_and_method()
{
Event::listen(TestEvent::class, TestListener::class);
Event::assertListening(TestEvent::class, [TestListener::class, 'hand*']);
}
public function test_wildcard_assertListening_class_only_vs_class_at_method()
{
Event::listen(TestEvent::class, [TestListener::class]);
Event::assertListening(TestEvent::class, TestListener::class . '@hand*');
}
public function test_wildcard_assertListening_class_list_vs_class_and_method()
{
Event::listen(TestEvent::class, [TestListener::class]);
Event::assertListening(TestEvent::class, [TestListener::class, 'hand*']);
}
public function test_wildcard_assertListening_class_list_vs_class_at_method()
{
Event::listen(TestEvent::class, [TestListener::class]);
Event::assertListening(TestEvent::class, TestListener::class . '@hand*');
}
public function test_wildcard_assertListening_class_and_method_vs_class_and_method()
{
Event::listen(TestEvent::class, [TestListener::class, 'handle']);
Event::assertListening(TestEvent::class, [TestListener::class, 'hand*']);
}
public function test_wildcard_assertListening_class_and_method_vs_class_at_method()
{
Event::listen(TestEvent::class, [TestListener::class, 'handle']);
Event::assertListening(TestEvent::class, TestListener::class . '@hand*');
}
public function test_wildcard_assertListening_class_at_method_vs_class_and_method()
{
Event::listen(TestEvent::class, TestListener::class . '@handle');
Event::assertListening(TestEvent::class, [TestListener::class, 'hand*']);
}
public function test_wildcard_assertListening_class_at_method_vs_class_at_method()
{
Event::listen(TestEvent::class, TestListener::class . '@handle');
Event::assertListening(TestEvent::class, TestListener::class . '@hand*');
}
} /**
* Assert if an event has a listener attached to it.
*
* @param string $expectedEvent
* @param string|array $expectedListener
* @return void
*/
public function assertListening($expectedEvent, $expectedListener)
{
$normalizedListener = $this->parseListener($expectedListener);
foreach ($this->dispatcher->getListeners($expectedEvent) as $listenerClosure) {
$actualListener = $this->parseListener((new ReflectionFunction($listenerClosure))
->getStaticVariables()['listener']);
if ($actualListener === $normalizedListener ||
($actualListener instanceof Closure &&
$normalizedListener === Closure::class) ||
(is_array($actualListener) && is_array($normalizedListener) &&
count($actualListener) === count($normalizedListener) &&
count($actualListener) === 2 &&
$actualListener[0] === $normalizedListener[0] &&
Str::is($normalizedListener[1], $actualListener[1])))
{
PHPUnit::assertTrue(true);
return;
}
}
PHPUnit::assertTrue(
false,
sprintf(
'Event [%s] does not have the [%s] listener attached to it',
$expectedEvent,
print_r($expectedListener, true)
)
);
}
protected function parseListener($listener)
{
if (is_array($listener) && count($listener) == 1 && is_string($listener[0])) {
$listener = $listener[0];
}
if (is_string($listener)) {
if (Str::contains($listener, '@')) {
$listener = Str::parseCallback($listener);
} else {
$listener = [$listener, 'handle'];
}
}
return $listener;
} |
Happy to submit the above as a code/test PR and as a Docs PR - just advise which version(s) of Laravel you would like these to apply to (and if multiple versions your preferred way for me to submit PRs to them all). |
Hi @Sophist-UK @crynobone created PR #54907 Edited: I have used your suggestion code and written test case too , credits to @Sophist-UK |
@pandiselvamm I really do NOT appreciate your rank plagiarism of my own hard work!!!! This is both disrespectful and contrary to both the Github and Laravel codes of conduct, and indeed breach of copyright since unless I explicitly say otherwise, my code is Copyright me and I have not given you permission to use it in a PR. |
Sorry @Sophist-UK , I tried to add you as co author in it just raised your pr with suggestion code here I won't taken your credits but sorry about that , you can see my commit time I have committed before you mentioned that I going to raise PR but I tried to add you as a authour It was done mistakenly apologize for that |
Perhaps this is a mistake of a relative novice - or perhaps it is a sign of a newcomer trying to establish a history of open source contributions to boost his credentials and help his career by stealing the efforts of others.. A few minutes ago I was able to see your contribution history but NOW it is hidden - so apparently you have now chosen to hide this from me which is NOT a sign of someone repentant about an honest mistake. If you don't want to get a PUBLIC reputation for stealing other people's efforts, then you need to stop doing it and be more genuinely apologetic. |
@pandiselvamm More to the point, @taylorotwell has rejected the PR you submitted, perhaps because you didn't justify it well enough. So not only did you steal my efforts, but you have also caused them to be rejected. So now, having already spent quite a lot of effort finding a workaround for my PR elsewhere that ran into this problem, and then spending effort in writing a set of tests, and then more effort in writing a solution, I now have to spend even more of my time attempting to undo the damage you have done. |
@pandiselvamm I would suggest that in future you limit writing PRs for other people's issues to those which have the "Help Wanted" tag. |
Okay sure @Sophist-UK , it was done mistakenly if am doing this again I will try to add co author my code also used as co author I thought like that we can do but I can't In future I will follow this , I apologise for my mistake for not added as a author on to it |
This has got to be the least sincere apology ever offered - not a single apology for stealing my code and then rendering my efforts wasted - just a statement that next time you steal my code you will tell everyone that you co-wrote it. I suggest that you book yourself onto an Ethics 101 course - because right now you seem to me to be completely lacking in them. |
@Sophist-UK Hi, I sincerely apologize for using your suggested code and raising the PR without seeking your permission. It was a mistake on my part, and I truly regret the oversight. I'll ensure this never happens again. |
@pandiselvamm So what efforts are you now going to make to redress the situation you have created where my efforts have been wasted? Are you going to go cap in hand to Taylor and tell him how you stole my code and screwed up and ask him "pretty please" to reconsider my own PR if I submit it? |
@Sophist-UK , Please feel free to submit a new PR with those changes, and I will ask Taylor to review it again. Additionally, we should also request an update in the documentation |
@pandiselvamm That isn't what I asked you to do. Once Taylor rejects a PR, I do not think it appropriate to submit another one without knowing he will consider it. You created this situation - you caused my efforts to be completely wasted - so IMO you now need to use your own efforts to try to put things right and not just walk away without needing to work at this. YOU need to go cap-in-hand to Taylor and explain the situation and see whether he will consider a PR if I now submit one. You might even find this cathartic. |
@Sophist-UK Please raise a new PR, I will provide the needed comments for the request to Taylor to re consider the PR. |
@pandiselvamm Having now submitted a PR, I note that in addition to stealing my code and representing it as your own, the PR you submitted was sub-standard in three ways:
So not only were your ethics non-existent, but you technical approach was lazy and sloppy. Shame on you!!! |
@crynobone I have now submitted a PR, and I hope that Taylor will give it normal consideration and not rejected it out of hand as a duplicate of #54907 (which it isn't). The new PR has reduced functionality cf. the above code and simply fixes the bug in the existing code and does not introduce new functionality such as expected listener wildcards (which might be nice to have but should be in a separate PR for separate consideration). The new PR also fixes a further bug in the code related to closures, and has tests which both demonstrate the issue and show it fixed. |
@pandiselvamm wrote:
Well, I did indeed submit a new PR over a week ago, which you almost certainly spotted, but which I nevertheless pointed out here, and yet you have not added a comment to apologise to @taylorotwell and request him to re-consider my PR as you promised to do. Your word is clearly worth nothing more than your previous apology i.e. absolutely nothing. Shame on you!!! |
Laravel Version
9.x, 10.x, 11.x, 12.x
PHP Version
All supported
Database Driver & Version
None
Description
As far as I can tell this is an issue with all versions of Laravel back to at least v9.
The documentation at https://laravel.com/docs/events#manually-registering-events says that you should manually associate events and listeners in your service provider like this:
however the deprecated
EventFaker
documentation says:The code in v9.x
framework/src/Illuminate/Support/Testing/Fakes/EventFake.php
clearly suggests that you can specify listeners as either"class@method"
or["class", "method"]
.It turns out that there are (IMO) three algorithmic bugs in the
assertListening
method:Solution
EventFaker
documentation from 8.x to later versions."class@method"
.Steps To Reproduce
https://github.com/laravel/framework/blob/9.x/src/Illuminate/Support/Testing/Fakes/EventFake.php#L90-L103 are:
and IMO should be:
The text was updated successfully, but these errors were encountered: