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

[11.x] Adding minRatio & maxRatio rules on Dimension validation ruleset #52482

Merged
merged 10 commits into from
Sep 5, 2024

Conversation

CamKem
Copy link
Contributor

@CamKem CamKem commented Aug 14, 2024

This PR introduces the ability to set a minimum & maximum aspect ratio in the Dimension rule.

Currently the framework only supports fixed ratios within the rule, allowing restriction of aspect ratios to only the defined values, this would allow the user to set a range of aspect ratios that an image would have to fall within.

The premise for this was conceived whilst I was doing further work on the image upload feature for Pinkary. We are have been considering how to deal with very long images, such as screenshots of full webpages taken using devtools, where the aspect ratio is on the extreme end & it would be nice to be able to set a range to handle this through validation within the framework.

Currently we would need to write a custom rule or use a closure like so:

function ($attribute, $value, $fail) {
    [$width, $height] = getimagesize($value->getPathname());
    $aspectRatio = $width / $height;
    if ($aspectRatio > 1 / 2 || $aspectRatio < 1 / 3) {
        $fail("The image aspect ratio must be between 1:2 and 1:3.");
    }

This PR introduces the ability to use it like so:

File::image()
    ->types(['jpeg', 'png', 'gif', 'webp', 'jpg'])
    ->max($this->maxFileSize)
->dimensions(Rule::dimensions()
    ->minRatio(1/2)
    ->maxRatio(1/3)
),

You could use only the minimum or maximum ratio alone, if you don't need upper or lower limits.

I also added a range method, that on evaluations adds a min_ratio and max_ratio constraint.

File::image()
    ->types(['jpeg', 'png', 'gif', 'webp', 'jpg'])
    ->max($this->maxFileSize)
->dimensions(Rule::dimensions()
    ->ratioRange(min: 1/2, max: 1/3)
),

All 3 of the added methods would be a good edition to the Validation capabilities of the framework.

I have added tests to cover the addition of these methods to the Dimension ruleset.

Screenshot 2024-08-14 at 11 56 19 PM

@CamKem CamKem changed the title Feat: Extended ratio rules on Dimension validation ruleset [11.x] Feat: Extended ratio rules on Dimension validation ruleset Aug 15, 2024
@CamKem CamKem changed the title [11.x] Feat: Extended ratio rules on Dimension validation ruleset [11.x] Adding minRation & maxRatio rules on Dimension validation ruleset Aug 15, 2024
@CamKem CamKem changed the title [11.x] Adding minRation & maxRatio rules on Dimension validation ruleset [11.x] Adding minRatio & maxRatio rules on Dimension validation ruleset Aug 15, 2024
@taylorotwell
Copy link
Member

Please add a translation line for this and then mark ready for review again. 👍

@taylorotwell taylorotwell marked this pull request as draft August 16, 2024 18:17
@CamKem
Copy link
Contributor Author

CamKem commented Aug 28, 2024

@taylorotwell I took your meaning as wanting me add a more granular translation line for the added parameters, however based on the existing implementation of this Dimension rule, without a lot of additional changes we can only utilise the current translation line in validation.php

    'dimensions' => 'The :attribute field has invalid image dimensions.',

We can then use the parameters within the string, which are swapped out for their values in FormatsMessage parser: Currently :width, :min_width, :max_width, :height, :min_height, :max_height & :ratio, Then If you merge this PR the :min_ratio & :max_ratio will also be available.

I spent quite a while looking into & working on refactoring the Dimensions rule & related logic, to bring it inline with the granularity of validation messages afforded to us by the similar File rule, whilst also ensuring there are no breaking changes. It's possible & I have a patch with a lot of the work for an implementation, but will require further consideration, such as how the between rules will interact with the min/max, etc...

Rather than go into it further & then to change the scope of this PR, I believe it would be wise to work on a refactor in a seperate PR that is independent of this one.


Happy to hear your thoughts.

@CamKem
Copy link
Contributor Author

CamKem commented Aug 28, 2024

Tested this validation rule / message combo

 $this->validate(
            rules: [
                'images.*' => [
                    Rule::dimensions()
                        ->maxHeight(4000)
                        ->maxWidth(4000)
                        ->ratioBetween(min: 1 / 2, max: 2 / 5),
                ],
            messages: [
                'images.*.dimensions' => 'The image must be a maximum of :max_width x :max_height pixels & ratio between :min_ratio and :max_ratio.',
            ]
        );
Screenshot 2024-08-28 at 11 05 05 PM

I'll start working on a seperate branch to rewrite the rule to facilitate this kind of granularity. (Pending further comment)

[
   'images.*.max_width' => 'The image width may not be greater than :max_width pixels.',
   'images.*.max_height' => 'The image height may not be greater than :max_height pixels.',
   'images.*.height_between' => 'The image height must be between :min_height and :max_height pixels.',
   'images.*.min_ratio' => 'The image aspect ratio must be at least 1/2.',
   'images.*.max_ratio' => 'The image aspect ratio must be less than 2/5.',
   'images.*.ratio_between' => 'The image aspect ratio must be between 1/2 and 2/5.',
   // Providing default translation line messages for the following:
   // width, min_width, max_width, width_between.
   // height, min_height, max_height, height_between
   // ratio, min_ratio, max_ratio, ratio_between

@CamKem CamKem marked this pull request as ready for review August 28, 2024 13:36
@taylorotwell taylorotwell merged commit ffa4d15 into laravel:11.x Sep 5, 2024
29 checks passed
@taylorotwell
Copy link
Member

Got it - thanks.

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