-
Notifications
You must be signed in to change notification settings - Fork 99
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
Cleanup and deprecate confusing annotations #583
Comments
We do use On the Also, if there are no plans to allow fields to be resolver-dynamic (i.e. deciding whether to hide it on every call to ->resolve), it might make sense to just deprecate it altogether to avoid confusion. On
class NotePolicy
{
public function create(User $authenticated): Response
{
return $this->allowWhenAdmin($authenticated) ??
$this->denyWhenOverTheLimit($authenticated) ??
Response::allow();
}
public function update(User $authenticated, Note $note): Response
{
return $this->allowWhenAdmin($authenticated) ??
$this->denyWithoutAccess($authenticated, $note) ??
Response::allow();
}
} then you map your entity to a policy ( $gate->forUser($auth)->authorize('create', Note::class);
$gate->forUser($auth)->authorize('update', $note) The same thing is easily applied on input/query fields: #[Input]
class Something {
#[Field]
#[Right('view')] // <-- call ->authorize('view', $this->otherEntity)
public OtherEntity $otherEntity;
} So from Laravel's point of view, |
It works the same in symfony |
@oprypkhantc I agree on the Symfony expression language. Luckily in PHP 8.5, support for closures will be there and it's already merged in. https://wiki.php.net/rfc/closures_in_const_expr I propose that we refactor the -- As for If we only hide types/fields when unauthorized, and not based on a "right", we could potentially cache 2 versions of the schema, in this case, and deliver back the appropriate one. However, with the
As for So, after these changes, we'd be able to cache one or two versions of the schema.
In addition to these changes, we could actually add another attribute, |
Having closures in attributes is awesome. Finally can we have custom inline validation in attributes. I'm also amazed that PHP is one of the first languages (the first?) to allow this :) On other attributes:
As always, I'm advocating for something like FrankenPHP :) It's pretty trivial to setup and gains you so much performance. |
The following annotations are confusing and have a lot of overlap in their documented functionality.
#[Logged]
#[Right]
#[HideIfUnauthorized]
Personally, we don't use any of them and I find the
#[Security]
annotation to be more than appropriate, especially with the expression language extensibility. Nonetheless, we're left with these others in the library. This ticket is to open a discussion into the current use of these annotations and potentially clean them up and deprecate where possible.Further,
#[HideIfUnauthorized]
, is dynamic in nature, causing the inability to cache types/schema for performance purposes. See #569 for more discussion on this topic.As I understand it,
#[HideIfUnauthorized]
is required to hide the field, regardless of#[Logged]
or#[Right]
being declared. In this case, I don't see why#[Logged]
and#[Right]
cannot implement an argument that determines if the field is hidden or not:#[Logged(hide: true)]
. TBH, I don't even see the purpose of#[Logged]
or#[Right]
, if the field isn't being hidden. So, why aren't these hiding by default and what are they doing when declared alone?Additionally, can
#[Logged]
and#[Right]
be merged together, and/or replaced by#[Security]
?All of these annotations are terribly confusing and the design is pretty awful IMO. I'd like to hear from others in how you're using them, suggestions for improvements, but with the overall goal of cleaning these up and deprecating them as much as possible.
The text was updated successfully, but these errors were encountered: