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

[9.x] Correct channel matching #44531

Merged
merged 1 commit into from
Oct 10, 2022
Merged

[9.x] Correct channel matching #44531

merged 1 commit into from
Oct 10, 2022

Conversation

taylorotwell
Copy link
Member

@taylorotwell taylorotwell commented Oct 10, 2022

Laravel previously had some unexpected behavior (imo) when matching channels. This was raised to me via email by a community member.

If you have:

Broadcast::channel(
    'App.Interaction.{uuid}',
    static function ($user, $uuid) {
        return true;
    },
);

And you attempt to subscribe to channel App.Interaction.some-uuid.Internal, the channel will match; however, the injected parameter will just be "some-uuid"... ".Internal" will be removed. This is because the channel matching in Laravel was using a fairly naive Str::is check which did not match the regular expression logic of the parameter injection. IMO the logic should be the same between the two. So, I have updated the channel matching regular expression to match the parameter injection regular expression.

In theory this could break applications that expected this route to match; however, those routes were already receiving a sliced parameter that may not be what they expect. So, I am viewing this as a bug. Will revisit if community feedback pushes back on release.

There was a test that showed this broken behavior but it appears to be added just out of thoroughness by a community member 4 years ago when the test was first added. Not for a specific use case.

@taylorotwell taylorotwell merged commit d013c86 into 9.x Oct 10, 2022
@taylorotwell taylorotwell deleted the channel-matching branch October 10, 2022 14:24
BrandonSurowiec added a commit to BrandonSurowiec/framework that referenced this pull request Oct 10, 2022
Also removed the `Illuminate\Support\Str` import as there are no more references to that class.
taylorotwell pushed a commit that referenced this pull request Oct 10, 2022
Also removed the `Illuminate\Support\Str` import as there are no more references to that class.
@PhiloNL
Copy link
Contributor

PhiloNL commented Oct 11, 2022

This did "break" in one of my apps by throwing 403 errors, but I agree this is a bug.
I did not fully understand the description at first so if anyone is looking for another example:

Broadcast::channel('team.{team}', function ($user, App\Models\Team $team) {
    return (int) $user->currentTeam->id === (int) $team->id;
});

// Subscribing to the following channel would work in < v9.35.0
// team.{$teamId}.products.{$productId}
// As a partial match was found for "team.{$teamId}" and ".products.{$productId}" is ignored.

// In => v9.35.0, you have to explicitly define the full route, so besides the channel above, you would need the following:
Broadcast::channel('team.{team}.products.{product}', function ($user, App\Models\Team $team, \App\Models\Product $product) {
    return (int) $user->currentTeam->id === (int) $product->team_id;
});

@GrahamCampbell GrahamCampbell changed the title Correct channel matching [9.x] Correct channel matching Nov 6, 2022
@verner-lall
Copy link

This change causes customer.order.1 to match channel order.{orderId}

Is that expected behaviour?

@timacdonald
Copy link
Member

I have created a PR to address this: #45692

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.

4 participants