roadrunner-php / laravel-bridge

πŸŒ‰ RoadRunner ⇆ Laravel bridge πŸ‡ΊπŸ‡¦β€οΈ
https://roadrunner.dev/docs/integration-laravel
MIT License
372 stars 25 forks source link

Compatibility with Telescope #28

Closed tristanpemble closed 3 years ago

tristanpemble commented 3 years ago

Describe the bug

Laravel Telescope's service provider accesses the current request in boot() to determine if this request should be recorded. Trace below:

At this point the request object is not bound. The request will default resolve to a root path (/). And since the provider is not reset on each request by default, this means every request will be logged, ignoring the Telescope request filters. This is the cause of the issue reported here: https://github.com/avto-dev/roadrunner-laravel/issues/23

Configuring TelescopeServiceProvider to reset via the reset_providers config does not fix this issue. By default, providers are reset during BeforeLoopIterationEvent, and the BindRequestListener is run afterward, during the BeforeRequestHandlingEvent. This means that even though the provider will get reset, since the request is resolved during the boot() call, the path seems to always resolve to /.

I attempted to create my own ResetTelescopeListener and fire it after the BindRequestListener is called:

class ResetTelescopeListener implements ListenerInterface
{
    public function handle($event): void
    {
        if ($event instanceof WithApplication) {
            $app = $event->application();

            $provider = new \Laravel\Telescope\TelescopeServiceProvider($app);
            $provider->register();
            $app->call([$provider, 'boot']);
        }
    }
}

However, this still doesn't seem to fix the issue. It's not clear to me why. It seems like the provider is getting initialized twice on every request.

I'm not sure if there is a path forward without requesting a change on their end.

Expected behaviour

There is a clearly defined path for using Laravel Telescope with roadrunner and roadrunner-laravel.

Actual behaviour

It is unclear how to use Laravel Telescope with roadrunner and roadrunner-laravel.

Steps to reproduce

  1. Install Laravel Telescope, roadrunner, and roadrunner-laravel
  2. In the Laravel Telescope requests dashboard, observe that all requests to /telescope are being logged, ignoring the default filters defined here.

System information

Key Value
PHP version 8.0
Current package version 3.7.0
RoadRunner version 1.8.3
Environment Docker

RoadRunner configuration file content

http:
  address: 0.0.0.0:80
  workers:
    command: 'php ./vendor/bin/rr-worker'

static:
  dir: 'public'

reload:
  enabled: false

Package configuration file content

Defaults with changes to debug described above.

Additional context

N/A

tarampampam commented 3 years ago

Hi @tristanpemble!

Request object usage in boot() method is very bad practice.. Anyway, we can try to fix it (move Telescope::start from boot() method into custom event listener):

{
  "extra": {
    "laravel": {
      "dont-discover": [
        "laravel/telescope"
      ]
    }
  }
}
<?php // File: ./app/TelescopeServiceProvider.php

namespace App;

use Laravel\Telescope\Telescope;
use Illuminate\Support\Facades\Route;

class TelescopeServiceProvider extends \Laravel\Telescope\TelescopeServiceProvider
{
    /**
     * {@inheritdoc}
     */
    public function boot()
    {
        if (! config('telescope.enabled')) {
            return;
        }

        Route::middlewareGroup('telescope', config('telescope.middleware', []));

        $this->registerRoutes();
        $this->registerMigrations();
        $this->registerPublishing();

        //Telescope::start($this->app); // disabled
        Telescope::listenForStorageOpportunities($this->app);

        $this->loadViewsFrom(
            __DIR__.'/../resources/views', 'telescope'
        );
    }
}
<?php // File: ./config/app.php

return [
    'providers' => [
        // your app providers goes here
        App\TelescopeServiceProvider::class, // added
    ],
];
<?php // File: ./app/StartTelescopeListener.php

namespace App;

class StartTelescopeListener implements \Spiral\RoadRunnerLaravel\Listeners\ListenerInterface
{
    /**
     * {@inheritdoc}
     */
    public function handle($event): void
    {
        if ($event instanceof \Spiral\RoadRunnerLaravel\Events\Contracts\WithApplication) {
            \Laravel\Telescope\Telescope::start($event->application());
        }
    }
}
<?php // File: ./config/roadrunner.php

use Spiral\RoadRunnerLaravel\Events;
use Spiral\RoadRunnerLaravel\Listeners;

return [
    'listeners' => [
        // ...

        Events\BeforeRequestHandlingEvent::class => [
            Listeners\RebindRouterListener::class,
            Listeners\InjectStatsIntoRequestListener::class,
            Listeners\BindRequestListener::class,
            Listeners\ForceHttpsListener::class,
            Listeners\SetServerPortListener::class,

            App\StartTelescopeListener::class, // added
        ],

    // ...
];

I think, in this case request object will be correct. Please, give me your feedback (code was not tested by me).

tarampampam commented 3 years ago

@tristanpemble Issue can be closed?

tarampampam commented 3 years ago

Closed due inactivity.