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] Use command string instead of array on Concurrency\ProcessDriver #52813

Merged
merged 2 commits into from
Sep 16, 2024

Conversation

rodrigopedra
Copy link
Contributor

This is a follow-up to PR #52807

This PR:

  • Builds a command string to call the invoke-serialized-closure artisan command, instead of using an array of commands parts

  • Changes the phpBinary and artisanBinary methods to use the same implementation as Illuminate\Console\Application, which already includes escaping of these binary paths

    public static function phpBinary()
    {
    return ProcessUtils::escapeArgument((new PhpExecutableFinder)->find(false) ?: 'php');
    }

    public static function artisanBinary()
    {
    return ProcessUtils::escapeArgument(defined('ARTISAN_BINARY') ? ARTISAN_BINARY : 'artisan');
    }

Notes

The issue with using the command as an array, is that Symfony's Process Component will quote each part when escaping the command, thus not allowing to send a process to the background by using 2>&1 &

Previous PR fixed issue #52790, but kept the calling process alive, as noted in comment #52807 (comment)

This approach allows the command to be truly sent in the background without holding up an FPM worker until the deferred commands are executed

Post Script

I noted the Concurrency component's composer.json file is missing illuminate/support in its require section. Also, the namespace on the PSR-4 autoload section is wrong.

"require": {
"php": "^8.2",
"illuminate/process": "^11.0",
"laravel/serializable-closure": "^1.2.2"
},
"autoload": {
"psr-4": {
"Illuminate\\Support\\": ""
}
},

Instead of modifying it in this PR, or sending it as a new PR, I made comments to PR #52801 which is refactoring some related classes.

@rodrigopedra
Copy link
Contributor Author

I tested with this modified code from issue #52790

<?php

use Illuminate\Support\Facades\Artisan;
use Illuminate\Support\Facades\Concurrency;
use Illuminate\Support\Sleep;

Artisan::command('testing', function (): void {
    Concurrency::defer(function () {
        Sleep::for(3)->seconds();
        info('a');
    });

    $this->info('b');
});

And it works perfectly.

The calling artisan command ends, returns to the prompt, and then 10 seconds later the deferred process writes to the log, as originally expected.

@hafezdivandari
Copy link
Contributor

You may use Illuminate\Console\Application::formatCommandString()

/**
* Format the given command as a fully-qualified executable command.
*
* @param string $string
* @return string
*/
public static function formatCommandString($string)
{
return sprintf('%s %s %s', static::phpBinary(), static::artisanBinary(), $string);
}

@rodrigopedra
Copy link
Contributor Author

Also tested with this route:

<?php

use Illuminate\Support\Facades\Concurrency;
use Illuminate\Support\Facades\Route;
use Illuminate\Support\Sleep;

Route::get('/', function () {
    Concurrency::defer(function () {
        Sleep::for(3)->seconds();
        \info('a');
    });

    return 'b';
});

On a nginx/PHP-FPM process, and it also executes as expected.

@rodrigopedra
Copy link
Contributor Author

@hafezdivandari I thought on using it, but I feared adding a new dependency to the Concurrency component's composer.json.

Although IMO, we could centralize all these calls somewhere, maybe on the Support component which already has a ProcessUtils class.

@hafezdivandari
Copy link
Contributor

@rodrigopedra #52744 (comment)

@rodrigopedra
Copy link
Contributor Author

@hafezdivandari done!

Thanks for reviewing it =)

@taylorotwell taylorotwell merged commit fc71e91 into laravel:11.x Sep 16, 2024
31 checks passed
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.

3 participants