Skip to content

refactor: Some minor performance & readability enhancements #53596

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Nov 21, 2024
Merged

refactor: Some minor performance & readability enhancements #53596

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 4 additions & 14 deletions src/Illuminate/Bus/Dispatcher.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,9 +109,7 @@ public function dispatchNow($command, $handler = null)
{
$uses = class_uses_recursive($command);

if (in_array(InteractsWithQueue::class, $uses) &&
in_array(Queueable::class, $uses) &&
! $command->job) {
if (isset($uses[InteractsWithQueue::class], $uses[Queueable::class]) && ! $command->job) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@ralphjsmit ralphjsmit Nov 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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))

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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();
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

$command->setJob(new SyncJob($this->container, json_encode([]), 'sync', 'sync'));
}

Expand Down Expand Up @@ -217,7 +215,7 @@ public function dispatchToQueue($command)
{
$connection = $command->connection ?? null;

$queue = call_user_func($this->queueResolver, $connection);
$queue = ($this->queueResolver)($connection);

if (! $queue instanceof Queue) {
throw new RuntimeException('Queue resolver did not return a Queue implementation.');
Expand All @@ -239,19 +237,11 @@ public function dispatchToQueue($command)
*/
protected function pushCommandToQueue($queue, $command)
{
if (isset($command->queue, $command->delay)) {
return $queue->laterOn($command->queue, $command->delay, $command);
}

if (isset($command->queue)) {
return $queue->pushOn($command->queue, $command);
}

if (isset($command->delay)) {
return $queue->later($command->delay, $command);
return $queue->later($command->delay, $command, queue: $command->queue ?? null);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

return $queue->push($command);
return $queue->push($command, queue: $command->queue ?? null);
}

/**
Expand Down
2 changes: 1 addition & 1 deletion tests/Bus/BusDispatcherTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ public function testCommandsThatShouldQueueIsQueuedUsingCustomQueueAndDelay()
$container = new Container;
$dispatcher = new Dispatcher($container, function () {
$mock = m::mock(Queue::class);
$mock->shouldReceive('laterOn')->once()->with('foo', 10, m::type(BusDispatcherTestSpecificQueueAndDelayCommand::class));
$mock->shouldReceive('later')->once()->with(10, m::type(BusDispatcherTestSpecificQueueAndDelayCommand::class), '', 'foo');

return $mock;
});
Expand Down