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] Map integer parameter to parameter name when resolving binding field #42571

Merged
merged 1 commit into from
May 31, 2022

Conversation

ksassnowski
Copy link
Contributor

Fixes #42541

First of all, I'm really sorry about all the issues the initial PR has caused 😞

Summary

This PR reverts most of the changes of the original PR (#42425) while still fixing the bug described in the PR. The only method that was changed was the bindingFieldFor method of the Route class. All changes to ImplicitRouteBinding and RouteUri were reverted.

To fix the bug described in the original PR, we now resolve the parameter index to the parameter name by looking it up in the route's parameterNames array.

public function bindingFieldFor($parameter)
{
    if (is_int($parameter))  {
        $parameter = $this->parameterNames()[$parameter];
    }

    return $this->bindingFields[$parameter] ?? null;
}

This means that we no longer have to change the structure of the bindingFields array itself like in the original PR. The bug described in the related issue (#42541) was due to the fact that the ResourceRouteRegistrar also filled the route's binding fields with null values, just like my change in the original PR. The problem with that is that both of these implementations used null to signal the exact opposite of each other. My PR used null to mean "don't scope this parameter", whereas the ResourceRouteRegistrar used it to mean "scope this parameter but default to the model's route key name".

I left in all the additional tests I added since all the problems that got reported due to my original PR were not covered by any existing tests.

@taylorotwell
Copy link
Member

Thanks!

@mortenscheel
Copy link

FYI I think this PR introduced a breaking change in a rather exotic edge case. I posted a bug report (#42605), but I closed it myself when I realized that it only happens if you ask the route() helper to turn models into query params. Which is obviously a mistake, but up until v9.14 it produced valid URL's. Now it produces 500 errors.

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.

Scoped route binding in apiResource routes no longer working
3 participants