Skip to content
This repository was archived by the owner on Feb 7, 2024. It is now read-only.

Fix Symfony deprecation error in TriggerEventController #1147

Merged
merged 2 commits into from
Aug 30, 2023

Conversation

pajkho
Copy link

@pajkho pajkho commented Aug 1, 2023

Should fix #1146 that is caused by Symfony deprecating non-scalar values in InputBag.php.

@what-the-diff
Copy link

what-the-diff bot commented Aug 1, 2023

PR Summary

  • Modification in TriggerEventController.php
    The update made to this section of the code changes how the application identifies the 'channels' from the input or 'payload'. Previously, it used a default value if 'channels' wasn't presented inside the payload. Now, instead, it tries to get all values under 'channels' and only uses a default empty array, if 'channels' isn't there. This has the potential to make the usage of the information pulled from 'channels' more reliable.

@aalyusuf
Copy link

aalyusuf commented Aug 8, 2023

optional(..) should be removed as it will not work with foreach, so the fix should be:

$channels = $payload->all()['channels'] ?? [];

@ManuelLeiner
Copy link

ManuelLeiner commented Aug 10, 2023

optional(..) should be removed as it will not work with foreach, so the fix should be:

$channels = $payload->all()['channels'] ?? [];

Works for me.

After upgrading laravel/dusk from 7.9.1 to 7.9.2 via dependabot last week the websockets stopped working. I could verify with git bisect that this is the bad commit. Quickly changing that one line in the vendor files makes it work again.

Here is the list of updates that came in by merging the dependabot PR for laravel/dusk 7.9.2 and as we can see there are some symfony patches and a laravel/framework update.

Package operations: 1 install, 18 updates, 0 removals
  - Upgrading symfony/finder (v6.3.0 => v6.3.3): Extracting archive
  - Upgrading symfony/var-dumper (v6.3.1 => v6.3.3): Extracting archive
  - Upgrading symfony/css-selector (v6.3.0 => v6.3.2): Extracting archive
  - Upgrading symfony/routing (v6.3.1 => v6.3.3): Extracting archive
  - Upgrading symfony/process (v6.3.0 => v6.3.2): Extracting archive
  - Upgrading symfony/mime (v6.3.0 => v6.3.3): Extracting archive
  - Upgrading symfony/event-dispatcher (v6.3.0 => v6.3.2): Extracting archive
  - Upgrading symfony/http-foundation (v6.3.1 => v6.3.2): Extracting archive
  - Upgrading symfony/error-handler (v6.3.0 => v6.3.2): Extracting archive
  - Upgrading symfony/http-kernel (v6.3.1 => v6.3.3): Extracting archive
  - Upgrading symfony/string (v6.3.0 => v6.3.2): Extracting archive
  - Upgrading symfony/console (v6.3.0 => v6.3.2): Extracting archive
  - Upgrading symfony/translation (v6.3.0 => v6.3.3): Extracting archive
  - Upgrading league/mime-type-detection (1.11.0 => 1.12.0): Extracting archive
  - Upgrading nette/utils (v4.0.0 => v4.0.1): Extracting archive
  - Upgrading laravel/framework (v10.16.1 => v10.17.1): Extracting archive
  - Installing laravel/prompts (v0.1.3): Extracting archive
  - Upgrading phpunit/phpunit (10.2.6 => 10.2.7): Extracting archive
  - Upgrading laravel/dusk (v7.9.1 => v7.9.2): Extracting archive

@aalyusuf
Copy link

optional(..) should be removed as it will not work with foreach, so the fix should be:
$channels = $payload->all()['channels'] ?? [];

Works for me.

After upgrading laravel/dusk from 7.9.1 to 7.9.2 via dependabot last week the websockets stopped working. I could verify with git bisect that this is the bad commit. Quickly changing that one line in the vendor files makes it work again.

Here is the list of updates that came in by merging the dependabot PR for laravel/dusk 7.9.2 and as we can see there are some symfony patches.

Package operations: 1 install, 18 updates, 0 removals
  - Upgrading symfony/finder (v6.3.0 => v6.3.3): Extracting archive
  - Upgrading symfony/var-dumper (v6.3.1 => v6.3.3): Extracting archive
  - Upgrading symfony/css-selector (v6.3.0 => v6.3.2): Extracting archive
  - Upgrading symfony/routing (v6.3.1 => v6.3.3): Extracting archive
  - Upgrading symfony/process (v6.3.0 => v6.3.2): Extracting archive
  - Upgrading symfony/mime (v6.3.0 => v6.3.3): Extracting archive
  - Upgrading symfony/event-dispatcher (v6.3.0 => v6.3.2): Extracting archive
  - Upgrading symfony/http-foundation (v6.3.1 => v6.3.2): Extracting archive
  - Upgrading symfony/error-handler (v6.3.0 => v6.3.2): Extracting archive
  - Upgrading symfony/http-kernel (v6.3.1 => v6.3.3): Extracting archive
  - Upgrading symfony/string (v6.3.0 => v6.3.2): Extracting archive
  - Upgrading symfony/console (v6.3.0 => v6.3.2): Extracting archive
  - Upgrading symfony/translation (v6.3.0 => v6.3.3): Extracting archive
  - Upgrading league/mime-type-detection (1.11.0 => 1.12.0): Extracting archive
  - Upgrading nette/utils (v4.0.0 => v4.0.1): Extracting archive
  - Upgrading laravel/framework (v10.16.1 => v10.17.1): Extracting archive
  - Installing laravel/prompts (v0.1.3): Extracting archive
  - Upgrading phpunit/phpunit (10.2.6 => 10.2.7): Extracting archive
  - Upgrading laravel/dusk (v7.9.1 => v7.9.2): Extracting archive

optional(..) returns an object of type Illuminate/Support/Optional and is not iterable, so foreach will not work, you can test using :

        $arr = optional([1, 2]);
        var_dump(is_iterable($arr));

and optional() is never null so using it with null coalesce isn't correct either and an empty array will never be returned.

The error may be gone but the channels shouldn't receive anything.

@ManuelLeiner
Copy link

Sorry for the confusion. The line of code you suggested works. 👍

$channels = $payload->all()['channels'] ?? [];

I didn't try the optional.

Everything looks good now and happily updates via websockets in the frontend.

@ManuelLeiner
Copy link

@pajkho, thank you very much for your PR. Could you adapt your change to aalyusuf's suggestion? Then, I think we are one step closer for it to actually being merged and fixed.

$channels = $payload->all()['channels'] ?? [];

@ManuelLeiner
Copy link

It starts breaking with laravel/framework v10.17.0

laravel/framework#47914

@hamdallahjodah
Copy link

@ManuelLeiner Please any update on this?

@pajkho
Copy link
Author

pajkho commented Aug 13, 2023

Sorry for the late reply. I have updated the code now as suggested.

@ManuelLeiner
Copy link

@ManuelLeiner Please any update on this?

I am not really involved in this project.

@mpociot Moin Marcel, what do you think? Is it something that needs to be fixed here?

@gehrisandro
Copy link

@pajkho Thank you very much.

@mpociot This fixes the issue for me (using Laravel 9)

@ManuelLeiner
Copy link

A workaround for Laravel 10 is to downgrade the framework to v10.16.1.

composer require laravel/framework:10.16.1

You might have to downgrade some other dependencies, e.g. Jetstream, as well.

@ManuelLeiner
Copy link

@sschlein @mechelon, apologies for tagging you both. Is there any chance of resolving this problem?

@dbohn
Copy link

dbohn commented Aug 28, 2023

For anyone, who is still looking into how to fix this in their own application without waiting for a new release, the following approach could be used:

In your Laravel app, create a copy of the TriggerEventController.php class, in my case, I stored it in the App\Websockets\Server\Controllers namespace:

<?php

namespace App\Websockets\Server\Controllers;

use BeyondCode\LaravelWebSockets\Dashboard\DashboardLogger;
use BeyondCode\LaravelWebSockets\Facades\StatisticsLogger;
use BeyondCode\LaravelWebSockets\HttpApi\Controllers\Controller;
use Illuminate\Http\Request;

class TriggerEventController extends Controller
{
    public function __invoke(Request $request)
    {
        $this->ensureValidSignature($request);
        $payload = $request->json();

        if ($payload->has('channel')) {
            $channels = [$payload->get('channel')];
        } else {
            $channels = $payload->all()['channels'] ?? [];

            if (is_string($channels)) {
                $channels = [$channels];
            }
        }

        foreach ($channels as $channelName) {
            $channel = $this->channelManager->find($request->appId, $channelName);

            optional($channel)->broadcastToEveryoneExcept([
                'channel' => $channelName,
                'event' => $request->json()->get('name'),
                'data' => $request->json()->get('data'),
            ], $request->json()->get('socket_id'));

            DashboardLogger::apiMessage(
                $request->appId,
                $channelName,
                $request->json()->get('name'),
                $request->json()->get('data')
            );

            StatisticsLogger::apiMessage($request->appId);
        }

        return (object) [];
    }
}

Then we create a new Router class, that extends \BeyondCode\LaravelWebSockets\Server\Router and override the echo method, that registers the route for this endpoint. In the override, make sure to reference your own fixed version of the TriggerEventController class:

<?php

namespace App\Websockets\Server;

use App\Websockets\Server\Controllers\TriggerEventController;
use BeyondCode\LaravelWebSockets\HttpApi\Controllers\FetchChannelController;
use BeyondCode\LaravelWebSockets\HttpApi\Controllers\FetchChannelsController;
use BeyondCode\LaravelWebSockets\HttpApi\Controllers\FetchUsersController;
use BeyondCode\LaravelWebSockets\WebSockets\WebSocketHandler;

class Router extends \BeyondCode\LaravelWebSockets\Server\Router
{
    public function echo()
    {
        $this->get('/app/{appKey}', WebSocketHandler::class);

        $this->post('/apps/{appId}/events', TriggerEventController::class);
        $this->get('/apps/{appId}/channels', FetchChannelsController::class);
        $this->get('/apps/{appId}/channels/{channelName}', FetchChannelController::class);
        $this->get('/apps/{appId}/channels/{channelName}/users', FetchUsersController::class);
    }
}

Finally, in your AppServiceProvider's register method (or any other provider, that you find fitting), update the websockets.router binding to point to your own router implementation:

$this->app->singleton('websockets.router', function () {
    return new \App\Websockets\Server\Router();
});

Restart your websocket:serve command and you should be able to send broadcasts again!

@mpociot mpociot merged commit fee9a81 into beyondcode:1.x Aug 30, 2023
@mpociot
Copy link
Member

mpociot commented Aug 30, 2023

Thank you for the PR and sorry for the delay in merging this 🙈

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants