knuckleswtf / scribe

Generate API documentation for humans from your Laravel codebase.✍
https://scribe.knuckles.wtf/laravel/
MIT License
1.69k stars 302 forks source link

Url parameter access on formRequest does not work #594

Open oskobri opened 1 year ago

oskobri commented 1 year ago

Hello,

In a FormRequest, access directly to url parameter does not work (null). Here is a code example:

<?php

namespace App\Http\Requests;

use Illuminate\Foundation\Http\FormRequest;
use Illuminate\Support\Facades\Route;

class UserRequest extends FormRequest
{

    public function rules(): array
    {
        $this->user; // null

        $this->route('user'); // null

        Route::current()->parameters()['user']; // not null

    }
}

Only the third one works but i prefer to avoid this.

I ran php artisan scribe:generate

Dieter91 commented 1 year ago

Ran into the same problem here. Third option does indeed work.

MissaelAnda commented 1 year ago

Same here but not even the third option works. Im using ver. 4.12.0

shalvah commented 1 year ago

Thanks for all the reports. Help fixing this would also be appreciated.

AunCly commented 1 year ago

Hi,

Same problem here, i have a basic formRequest with the following rule :

Rule::unique('users', 'login')->ignore($this->user->login, 'login')

And scribe throw exception cause $this->user is null

I have dived into your package and started to write a solution. My solution work but only if i specify an @urlParam

For example : route PUT /api/users/{user} and controller :

/**
* @urlParam user int required L'identifiant de l'utilisateur. Exemple: 300
*/
public function update(UserRequest $request, UserModel $user){ // ... }

And here the following update that i have wrote into scribe Knuckles\Scribe\Extracting\Strategies\GetFromFormRequestBase.php

    public function __invoke(ExtractedEndpointData $endpointData, array $routeRules = []): ?array
    {
        return $this->getParametersFromFormRequest($endpointData, $endpointData->method, $endpointData->route);
    }

    public function getParametersFromFormRequest(ExtractedEndpointData $endpointData, ReflectionFunctionAbstract $method, Route $route): array
    {
        if (!$formRequestReflectionClass = $this->getFormRequestReflectionClass($method)) {
            return [];
        }

        if (!$this->isFormRequestMeantForThisStrategy($formRequestReflectionClass)) {
            return [];
        }

        $className = $formRequestReflectionClass->getName();

        /*
         * Get the params name/example value 
         */
        $params = [];
        foreach($endpointData->urlParameters as $param) {
            $params[$param->name] = $param->example;
        }

        /**
         * instanciate a new request with the route parameters
         */
        $request = \Illuminate\Http\Request::create($route->uri(), $route->methods()[0], $params);

        if (Globals::$__instantiateFormRequestUsing) {
            $formRequest = call_user_func_array(Globals::$__instantiateFormRequestUsing, [$className, $route, $method]);
        } else {
            /**
             * instanciate a new form request
             */
            $formRequest = \Illuminate\Foundation\Http\FormRequest::createFrom($request, new $className);
        }

        /**
         * bind the route parameters to the form request
         */
        $route->bind($formRequest);

        /**
         * set the parameters to the route
         */
        foreach($endpointData->urlParameters as $param) {
            $route->setParameter($param->name, $param->example);
        }

        /**
         * convert params to model instances
         */
        app('router')->substituteBindings($route);
        app('router')->substituteImplicitBindings($route);

        $formRequest->server->set('REQUEST_METHOD', $route->methods()[0]);

        $parametersFromFormRequest = $this->getParametersFromValidationRules(
            $this->getRouteValidationRules($formRequest),
            $this->getCustomParameterData($formRequest)
        );

        return $this->normaliseArrayAndObjectParameters($parametersFromFormRequest);
    }

But i have found 2 problems and thats why i don't PR your my solution. First : $endpointData->urlParameters have 2 keys, the one added by Strategies\UrlParameters\GetFromLaravelAPI::class and the second my @urlParams added by Strategies\UrlParameters\GetFromUrlParamTag::class, that don't cause problem but it's little strange that my @urlParams doesn't override the first one ?

Second : in the case i don't specify the @urlParams i have an not found exception throw by model binding cause GetFromLaravelAPI create a tag user_id and substitution doesn't work.

Hope it help. :)

shalvah commented 1 year ago

I've not looked at this in detail yet, but if you're unhappy with the results of GetFromLaravelAPI and want to adjust it, you can either:

  1. Remove the strategy from your config to disable it entirely, OR
  2. Use the normalizeEndpointUsing() hook to customise or disable URL normalization (the process that converts /{user} to /{user_id}). There are examples in the docs - https://scribe.knuckles.wtf/laravel/hooks#normalizeendpointurlusing
AunCly commented 1 year ago

I have already think to that, in fact i have no problem with this strategies, i just wanted to warn you about my fix problems, maybe i wasn't clear enough

MissaelAnda commented 1 year ago

Perhaps we can move normalizeEndpointUsing() to after extracting all data, and it would be nice if ExtractedEndpointData marked the urlParams example to Eloquent model binding so when extracting the body params it could try to do the models stratigies databaseFirst, factoryCreate or factoryMake.

mortenscheel commented 1 year ago

I had a similar issue, which I was able to solve by using instantiateFormRequestUsing.

Here's a simple example where the rules depend on a route param:

class SomeFormRequest extends FormRequest
{
    public Customer $customer;

    protected function prepareForValidation()
    {
        $this->customer = $this->route('customer');
    }

    public function rules()
    {
        return [
            'user_id' => ['required', Rule::exists('users', 'id')->where('customer_id', $this->customer->id)],
        ];
    }
}

And in AppServiceProvider's boot method:

if (class_exists(Scribe::class)) {
    Scribe::instantiateFormRequestUsing(function (string $formRequestClassName) {
        if ($formRequestClassName === SomeFormRequest::class) {
            $formRequest = new $formRequestClassName();
            $formRequest->customer = new Customer();
        }

        return $formRequest;
    });
}

Note that the Scribe documentation's example uses app()->makeWith() to instantiate the formrequest, which doesn't work in my experience. When a FormRequest is resolved this way, it automatically triggers the ValidatesWhenResolved trait, meaning that a ValidationException is thrown if the rules don't pass (which they probably won't since there are no request params). But you can instantiate the FormRequest using new, and then add "mock" dependencies, before passing it back to Scribe.

So basically this workaround works by having the dependencies defined as class attributes, which are normally populated in prepareForValidation(), but can also be set manually.

gabrielrbarbosa commented 1 year ago

@mortenscheel Thank you!! Using your solution solved #526

patrickomeara commented 10 months ago

I've just hit this issue after installing Scribe for the first time. I will look into a fix for this.

patrickomeara commented 10 months ago

I believe that in order to fix this issue, the path, annotated, attribute and method arguments should be merged and iterated over. If the types extend Model then instantiateExampleModel should be used to set the parameter on the route after it is bound using the request in getParametersFromFormRequest.

I'm still getting to know the codebase, for anyone experienced with the codebase does this sound like a suitable solution?

From what I can see the models are only instantiated for responses, would instantiating them for typed method arguments be a possibility?

shalvah commented 10 months ago

Not gonna lie, I'm thoroughly confused here...is the problem model binding or something else?

patrickomeara commented 10 months ago

Correct, model binding isn't working in rules()

I've added a test here

When in the rules() method, $this->route('post'), $this->post and Route::current()->parameters()['post'] should all return a post model. The first two return null, the latter returns a string.

shalvah commented 10 months ago

Ah, okay. Tbh, you have a better chance of fixing this than I do at the moment. 😅 Don't use Laravel much these days, and not been a fan of model binding tbh, especially within validation (and I'm also on vacation). PRs are welcome.

patrickomeara commented 10 months ago

Cool. I started hacking on it but need to get into the codebase a little more. I'll see what I can come up with.

igorsgm commented 5 months ago

I'm also with the same issue. Any luck here?

patrickomeara commented 5 months ago

Hey @igorsgm I haven't looked at this for a while, but it was a sticking point for us.

Have you tried the solution @mortenscheel mentioned?

igorsgm commented 5 months ago

I managed to fix it by creating a custom bodyParameter strategy: GetFromFormRequestExtended:

use Illuminate\Foundation\Http\FormRequest as LaravelFormRequest;
use Illuminate\Routing\Route;
use Knuckles\Camel\Extraction\ExtractedEndpointData;
use Knuckles\Scribe\Extracting\Shared\UrlParamsNormalizer;
use Knuckles\Scribe\Extracting\Strategies\BodyParameters\GetFromFormRequest;
use Knuckles\Scribe\Tools\Globals;

class GetFromFormRequestExtended extends GetFromFormRequest
{
    public function __invoke(ExtractedEndpointData $endpointData, array $routeRules = []): ?array
    {
        return $this->getParametersFromFormRequestWithBinding($endpointData, $endpointData->route);
    }

    /**
     * Overriding Knuckles\Scribe\Extracting\Strategies\GetFromFormRequestBase::getParametersFromFormRequest
     * to bind the request to the route and set the route parameters properly.
     */
    public function getParametersFromFormRequestWithBinding(ExtractedEndpointData $endpointData, Route $route): array
    {
        if (!$formRequestReflectionClass = $this->getFormRequestReflectionClass($endpointData->method)) {
            return [];
        }

        if (!$this->isFormRequestMeantForThisStrategy($formRequestReflectionClass)) {
            return [];
        }

        $className = $formRequestReflectionClass->getName();

        if (Globals::$__instantiateFormRequestUsing) {
            $formRequest = call_user_func_array(Globals::$__instantiateFormRequestUsing, [$className, $route, $endpointData->method]);
        } else {
            $formRequest = new $className;
        }

        // Set the route properly, so it works for users who have code that checks for the route.
        /** @var LaravelFormRequest $formRequest */
        $formRequest->setRouteResolver(function () use ($formRequest, $route, $endpointData) {
            // Also need to bind the request to the route in case their code tries to inspect current request
            $route = $route->bind($formRequest);

            // ADDING THIS LINE TO SET ROUTE PARAMETERS
            return $this->setRouteParametersAfterBinding($endpointData, $route);
        });

        $formRequest->server->set('REQUEST_METHOD', $route->methods()[0]);

        $parametersFromFormRequest = $this->getParametersFromValidationRules(
            $this->getRouteValidationRules($formRequest),
            $this->getCustomParameterData($formRequest)
        );

        return $this->normaliseArrayAndObjectParameters($parametersFromFormRequest);
    }

    /**
     * Dynamically sets route parameters for a given route based on URL parameters and method signatures.
     * It assigns parameters using the first instance of type-hinted Eloquent models, the first case of enums,
     * or a default example value.
     *
     * Note: Assumes model instances can be fetched with first() and enums are instantiated from their first case.
     */
    protected function setRouteParametersAfterBinding(ExtractedEndpointData $endpointData, Route $route): Route
    {
        $typeHintedEloquentModels = UrlParamsNormalizer::getTypeHintedEloquentModels($endpointData->method);
        $typeHintedEnums = UrlParamsNormalizer::getTypeHintedEnums($endpointData->method);

        foreach ($endpointData->urlParameters as $urlParameter) {
            $paramName = $urlParameter->name;

            $routeParameter = match (true) {
                array_key_exists($paramName, $typeHintedEloquentModels) => $typeHintedEloquentModels[$paramName]::first(),
                array_key_exists($paramName, $typeHintedEnums) => $typeHintedEnums[$paramName]->getCases()[0]?->getValue(),
                default => $urlParameter->example,
            };

            $route->setParameter($paramName, $routeParameter);
        }

        return $route;
    }
}

Then, inside scribe.php strategies:

        'bodyParameters' => [
            // Strategies\BodyParameters\GetFromFormRequest::class, --> Comment this line
            GetFromFormRequestExtended::class, // Add your new custom strategy
            Strategies\BodyParameters\GetFromInlineValidator::class,
            Strategies\BodyParameters\GetFromBodyParamAttribute::class,
            Strategies\BodyParameters\GetFromBodyParamTag::class,
        ],

After that, it worked as I intended.

coreymcmahon commented 1 month ago

Solution from @igorsgm above works, with one caveat (at least in our case). We also needed to disable endpoint URL normalisation using:

Scribe::normalizeEndpointUrlUsing(fn ($url) => $url);

This was because the route parameter was being normalised from the model name (eg: user) to id, which meant $typeHintedEloquentModels[$paramName] was empty.

Another solution here would be to simply inject all $typeHintedEloquentModels values as route parameters:

foreach ($typeHintedEloquentModels as $name => $model) {
    $route->setParameter($name, $model::first());
}

... but I'm not sure if there would be other implications with that approach.