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

Replace legacy league/pipeline dependency by standard Laravel Pipeline #229

Open
wants to merge 2 commits into
base: stable
Choose a base branch
from

Conversation

onlime
Copy link

@onlime onlime commented Jan 26, 2025

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!

@jhm-ciberman
Copy link

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

@onlime
Copy link
Author

onlime commented Jan 26, 2025

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.

@jhm-ciberman
Copy link

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.

@pataar
Copy link

pataar commented Jan 27, 2025

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants