browser-detect icon indicating copy to clipboard operation
browser-detect copied to clipboard

Replace legacy league/pipeline dependency by standard Laravel Pipeline

Open onlime opened this issue 10 months ago • 4 comments

I have removed the outdated and unmaintained league/pipeline and replaced it with standard Laravel Pipeline to avoid such deprecations in PHP 8.4:

   DEPRECATED  League\Pipeline\Pipeline::__construct(): Implicitly marking parameter $processor as nullable is deprecated, the explicit nullable type must be used instead in vendor/league/pipeline/src/Pipeline.php on line 18.

so the Parser::process() now looks like this:

    protected function process(string $agent): ResultInterface
    {
        return Pipeline::send(new Payload($agent))
            ->through([
                Stages\UAParser::class,
                Stages\MobileDetect::class,
                Stages\CrawlerDetect::class,
                Stages\DeviceDetector::class,
                Stages\BrowserDetect::class,
            ])
            ->then(fn (Payload $payload) => new Result($payload->toArray()));
    }

I had to change the return value logic in BrowserDetect, so it is acting the same as the other stage classes. To get all tests working, I had to pass the $next Closure to all those constructors, maybe not in the most elegant way.

This fixes #228

I cannot promise any further support for this package, as I have moved away to using plain matomo/device-detector with the built-in LaravelCache. I never needed more than basic browser client/OS detection, so this package was a complete overkill for my use case, with just too many dependencies. But thanks a lot for the great work!

onlime avatar Jan 26 '25 14:01 onlime

Maybe I am missing something, but where is the Laravel pipelines package being included in composer.json?

jhm-ciberman avatar Jan 26 '25 18:01 jhm-ciberman

Maybe I am missing something, but where is the Laravel pipelines package being included in composer.json?

illuminate/pipeline is now included in dbee9fc. I missed adding it, because anyway laravel/framework was already a dependency:

$ composer why laravel/framework
laravel/framework        v11.40.0 replaces  illuminate/pipeline (self.version)                     
orchestra/testbench      v9.9.0   requires  laravel/framework (^11.35.0)                           
orchestra/workbench      v9.13.1  requires  laravel/framework (^11.35)                             
(...)

so, unsure, if illuminate/pipeline really needs to be included as direct dependency.

onlime avatar Jan 26 '25 19:01 onlime

Yes, it needs to be defined. Because orchestra/testbench is only required as require-dev meaning it will be unavailable (and thus illuminate/pipeline) in production environments.

For example, when you deploy a PHP app, you usually use composer install --no-dev meaning that illuminate/pipeline will not be available.

jhm-ciberman avatar Jan 26 '25 21:01 jhm-ciberman

I removed the dependency and replaced it with a native array_reduce in https://github.com/hisorange/browser-detect/pull/230/files#diff-47bc8653b21716576958e25b6d7356ecb0f0070f17554d12d2bee985ac211b26

pataar avatar Jan 27 '25 13:01 pataar