refactor: Some minor performance & readability enhancements

[dshafik]

During my latest Inside Laravel livestream I noticed some minor inefficiencies in the Dispatcher code. These most likely are just older code that never got updated.

This includes 3 changes:

1. Useisset($uses[InteractsWithQueue::class], $uses[Queueable::class]) instead of a two expensive in_array() calls, this takes two O(n) operations to just O(1).
2. It replaces an expensive call_user_func() call with a dynamic method call, using ($this->queueResolver) to dererefence the property before calling the closure within.
3. Simplifies the pushCommandToQueue() implementation which previously checked if queue and/or delay were set and then calling pushOn() or laterOn() when with named params we can easily just call push() and later() — the use of queue: $command->queue ?? null rather than just queue: $command->queue is for readability. This one is the only one I'm concerned about, it's possible that an implementation of the Queue interface/child of the Queue abstract class has different behavior than just wrapping push() and later() — but as those must support the queue param I don't see any issue with it.

[milwad-dev]

The refactored version is harder to read than the previous version! (For me)

[Diddyy]

In contrast, I like it and can follow it better. I think it helps keep a clean code base so +1!

[crynobone]

Marking as draft since tests are failing.

[dshafik]

@crynobone all tests are passing now, only one minor change to a single test (mocking laterOn which is no longer called instead of later)

[GrahamCampbell]

This is an incorrect refactor. isset looks at the keys. in_array looks at the values.

[ralphjsmit]

Isn't class_uses_recursive() outputting the exact same key as value?

[
   SomeConcern::class => SomeConcern::class,
]

[dshafik]

@GrahamCampbell as @ralphjsmit points out, the result of class_uses_recursive() is a key-value pair with the trait name for both, therefore you can use isset() to check for the usage of a trait (O(1)) rather than traverse the array till you find the value using in_array(). (O(n))

[crynobone]

This should be fine and we use isset() as well for

framework/src/Illuminate/Foundation/Testing/Concerns/InteractsWithTestCaseLifecycle.php

Lines 196 to 220 in eb625fa

	$uses = array_flip(class_uses_recursive(static::class));
	if (isset($uses[RefreshDatabase::class])) {
	$this->refreshDatabase();
	}
	if (isset($uses[DatabaseMigrations::class])) {
	$this->runDatabaseMigrations();
	}
	if (isset($uses[DatabaseTruncation::class])) {
	$this->truncateDatabaseTables();
	}
	if (isset($uses[DatabaseTransactions::class])) {
	$this->beginDatabaseTransaction();
	}
	if (isset($uses[WithoutMiddleware::class])) {
	$this->disableMiddlewareForAllTests();
	}
	if (isset($uses[WithFaker::class])) {
	$this->setUpFaker();
	}

[dshafik]

@crynobone the array_flip() in that example should probably be removed too… (unrelated to this PR)

[crynobone]

Is it not possible to maintain using laterOn and pushOn for 11.x and change it for 12.x?

[dshafik]

@crynobone The methods aren't going anywhere, it's just unclear if there is any implementation that does anything except wrap the other functions.

[crynobone]

Yeah, just thinking if existing application using Bus::spy() and Bus::shouldHaveReceived('laterOn') and with these changes, it starts to cause failing tests.

[dshafik]

Happy to back that part out, although those feel like brittle tests I don't think it's good to break things for the sake of breaking things.