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

enhance: Add TARGET_PARAMETER to Attribute for improved Validation #523

Conversation

BadJacky
Copy link
Contributor

@BadJacky BadJacky commented Aug 4, 2023

Overview

This PR improves the attribute declaration in our classes by allowing annotations to be used on both properties and parameters. This enhancement provides additional flexibility in our coding practices, particularly for validation purposes.

Details

Previously, our attribute declaration only targeted properties, like so: #[Attribute(Attribute::TARGET_PROPERTY)]. This way of declaring attributes was missing an important aspect: validation via constructor parameters.

In cases where annotations were used in the constructor for parameters, PHPStan would throw an error, considering that the corresponding annotation doesn't have the appropriate rights to be used on parameters.

To resolve this issue and to increase the adaptability of our code, we have updated the attribute declaration: #[Attribute(Attribute::TARGET_PROPERTY | Attribute::TARGET_PARAMETER)]. This allows the use of annotations on parameters as well.

Impact

This change will help us:

  • Solve PHPStan errors related to annotation usage restrictions
  • Improve constructor-based validation
  • Make our classes more flexible and robust

Error Example

My PHP code

<?php

namespace App\Data;

use App\Models\User;
use Illuminate\Validation\Rule;
use Spatie\LaravelData\Attributes\Validation\Email;
use Spatie\LaravelData\Attributes\Validation\Exists;
use Spatie\LaravelData\Attributes\Validation\Max;
use Spatie\LaravelData\Attributes\Validation\Min;
use Spatie\LaravelData\Attributes\Validation\Nullable;
use Spatie\LaravelData\Attributes\Validation\RequiredUnless;
use Spatie\LaravelData\Attributes\Validation\StringType;
use Spatie\LaravelData\Data;
use Spatie\LaravelData\Support\Validation\ValidationContext;
use Symfony\Contracts\Service\Attribute\Required;

class LoginFormData extends Data
{
    public function __construct(
        #[
            Nullable,
            RequiredUnless('email', null),
            StringType,
            Min(5),
            Max(20),
            Exists(User::class, 'username'),
        ]
        public readonly string $username,
        #[
            Nullable,
            RequiredUnless('username', null),
            StringType,
            Min(5),
            Max(20),
            Email(Email::RfcValidation),
            Exists(User::class, 'email')
        ]
        public readonly string $email,
        #[
            Required,
            StringType,
            Min(5),
            Max(20)
        ]
        public readonly string $password,
    ) {
    }

    public static function rules(ValidationContext $context): array
    {
        return [
            'username' => ['required', 'string'],
            'password' => ['required', 'string'],
            'email'    => ['required', 'string', 'email', Rule::exists(User::class, 'email')],
        ];
    }
}

My PHPStan Configration

includes:
    - ./vendor/nunomaduro/larastan/extension.neon
    - ./vendor/phpstan/phpstan/conf/bleedingEdge.neon
parameters:
    editorUrl: 'phpstorm://open?file=%%file%%&line=%%line%%'
    phpVersion: 80200
    paths:
        - app/
        - tests/Support
    # Level 9 is the highest level
    level: 9

#    ignoreErrors:
#        - '#PHPDoc tag @var#'
#
    excludePaths:
        - tests/Support/Extends/
#
    checkMissingIterableValueType: false
    checkGenericClassInNonGenericObjectType: false

Error Images

Screenshot 2023-08-04 at 08 56 21 Screenshot 2023-08-04 at 08 56 34

We believe this update will greatly aid in maintaining clean, secure, and efficient code. As always, any feedback is welcome. Please review and let me know your thoughts.

@rubenvanassche
Copy link
Member

Thanks!

@rubenvanassche rubenvanassche merged commit 41b9d45 into spatie:main Aug 4, 2023
11 checks passed
@BadJacky BadJacky deleted the bugfix/update-attribute-target-property-parameter branch August 5, 2023 13:31
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.

2 participants