laravel-json-api / eloquent

Serialize Eloquent models to JSON API resources
MIT License
12 stars 15 forks source link

Make isReadOnly accesible without request #2

Closed martianatwork closed 3 years ago

martianatwork commented 3 years ago

Currently isReadOnly and other ReadOnly methods requires $request to be provided but believe this should work statically. Ex. Str::make('name')->readOnly() marks the field readOnly then when we do $field->isReadOnly() returns true I believe we should also provide more methods such as isReadOnlyOnCreate and isReadOnlyOnUpdate which will work without providing request

so we will have following

    private $readOnlyOnCreate = false;
    private $readOnlyOnUpdate = false;

   public function readOnlyOnCreate(): self
    {
        $this->readOnlyOnCreate = true;
        return $this;
    }

   public function isReadOnlyOnCreate(): self
    {
        return $this->readOnlyOnCreate;
    }

    public function isReadOnly($request = null): bool
    {
        if(empty($request)) return $this->readOnlyOnCreate || $this->readOnlyOnUpdate;
        return ($request->isMethod('PATCH') && $this->readOnlyOnUpdate) || ($request->isMethod('POST') && $this->readOnlyOnCreate);
    }    

Or please check this from Nova image

I personally prefer the first option as it is not dependent on the request parameter

This will allow us to use figure out if the field is readOnly while writing packages. or extending functionality.

lindyhopchris commented 3 years ago

The request is required because we're following Nova. As shown in your screenshot of the Nova code, the Nova code also expects the request when working out if a field is read-only. I can guarantee that if we do not provide the request to the callback, someone will raise an issue that it doesn't match Nova.

We've had issues in the past where people want the request for this kind of thing. For example, so that they can do this:

Str::make('email')->readOnly(
    fn($request) => !$request->user()->isAdmin()
);
martianatwork commented 3 years ago

@lindyhopchris I think you didnt get my point, request should be nullable as it will help when writing packages such as swagger implementation.

lindyhopchris commented 3 years ago

If analysing routes, it would be possible to pass in a request object that represents the Method/URI of the route that is being analysed. That way you'll get a more accurate answer than just passing in null.

lindyhopchris commented 3 years ago

In your example, this line:

if(empty($request)) {
    return $this->readOnlyOnCreate || $this->readOnlyOnUpdate;
}

would lead to inaccurate docs... it would say the field was always read-only, even if it was actually only read-only on one of create or update.

The docs would be more accurate if you passed in a request that represented the route you're analysing. Then you can show the field as read-only for either a create or an update request.

I would have thought that would be better?

lindyhopchris commented 3 years ago

Re-opening as the request parameter can now be optional. This isn't intended for docs - it's intended for if the developer is triggering hydration of a model outside of a HTTP request. I'd still recommend the for docs generation a request instance is passed in - a request that represents the request that is being analyzed for the docs.