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] Add support for granular messages on Dimensions rule #52707

Closed
wants to merge 22 commits into from

Conversation

CamKem
Copy link
Contributor

@CamKem CamKem commented Sep 9, 2024

Following the discussion in #52482, the goal of this PR is to facilitate the addition of granular error messages translation strings for more precise validation feedback for users.

The other fluent class validation rules, such as File & Password all follow a structure that implements DataAware & ValidatorAware, that facilitates the ability to provide granular validation messages for each of the conditions on the rules. As opposed to the Stringable rules (current how the Dimension rule structured), which are structured to be instantiated & then built into a validation string.

The Dimension rule has grown beyond the Stringable interface being an effective way to handle this rule. Currently having multiple chained fluent methods for the different conditions, means that when one of the conditions fails the whole rule fails, then it doesn't provide enough granularity in the validation feedback for the users & can be difficult for them to easily understand what the error is related to.

For example, currently if you utilise a fairly standard use where you want to provide a range of constraint on the dimension of an image like so:

Rule::dimension()->minWidth(100)->minHeight(100)->maxWidth(1000)->maxHeight(1000)->ratioBetween(5/2, 2/5)

You are only afforded the validation.dimension message to provide your users with feedback on why their image is failing validation. Which means you could set a custom message and utilise the :conditions_values to provide feedback, but this would be a very long string and as mentioned, without the ability to tell them which condition is causing the failure.

For example on the above rule, a custom message string would look something like this:

'images.*.dimension' => 'The image must be between :min_width x :min_height pixels and :max_width x :max_height pixels, and have an aspect ratio between :min_ratio and :max_ratio'

To achieve this granular feedback, this PR cleans up & enhances the Dimensions rule takes inspiration from structure of the File & Password fluent based rules, bringing uniformity across the way the logic in these rules are structured.

The features in the Password & File (and by extension ImageFile) rules which are implemented on Dimensions by this PR are, the default setup and ability to change it adding a callback, use of the Macroable trait & implementation of the methods for the Rule, DataAwareRule, ValidatorAwareRule interfaces.

Each set condition will now be individually checked in the ValidatesAttributes trait, providing a more useful and precise validation message on failure via the methods in the ReplacesAttributes trait.

This PR also adds heightBetween & widthBetween methods, to ensure that there is consistency for across the the conditions in the rule - width, height & ratio - each having the ability to set constraints & provide individual individual messages for fixed, min, max & between variations.

String based definitions of the dimensions rule are still supported with this change, as we funnelling the execution through the validatesDimensions method for each of methods which individually validate our constraints. Within this method, I have added the following logic to check for when it's a string based rule:

// This is to correctly parse string representations for dimensions
// like so "dimensions:min_width=100,min_height=200"
if (! Arr::isAssoc($parameters)) {
    $this->requireParameterCount(1, $parameters, 'dimensions');
    $parameters = $this->parseStringParameters($parameters);
}

Checking if it's an associative array, which how this PR builds the rules when used fluently. If it's not we pass the parameters array to $this->parseStringParameters($parameters) method to ensure that the array of conditions parsed into the correct format for processing by the validation logic to avoid any breaking changes in codebases which have defined the rule using the string format like so dimensions:width=100,height=100

// from this
[
  0 => "min_width=100"
  1 => "max_width=200"
  2 => "min_height=100"
  3 => "max_height=200"
  6 => "max_ratio=2/5"
];
// to this
[
  "min_width" => 100
  "max_width" => 200
  "min_height" => 100
  "max_height" => 200
  "max_ratio" => 2/5
];

The ValidationDimensionsRuleTest for the dimension rule has been modified to provide full coverage of the changes, and other tests adapted as needed.

Note on backward compatibility
Existing fluent or constructor passed array of constraints definitions will continue to function, however if the developer has set a custom message string for the dimensions rule, this PR will bypass it providing the default granular validation message translation strings defined in validation.php as feedback, unless the developer decides to add further custom ones. You might consider this to be unexpected behaviour.

I hope I've provided enough detailed information to make an informed decision if this is suitable for 11.x, or given the new behaviour, would be better suited to the next major version.

Screenshot 2024-09-09 at 11 26 26 PM

@CamKem
Copy link
Contributor Author

CamKem commented Sep 9, 2024

Other notes:

Initially I had it so if both a min & max condition are set, it's converted to a between validation check & message dynamically, however I reverted it as it figured that it's best to keep it verbose for now, to not introduce any unexpected behaviour.

I also looked into the option nesting the conditions like so:

'width' => [
    'fixed' => 'The :attribute field must be :width pixels.',
    'min' => 'The :attribute field must be at least :min_width pixels in width.',
    'max' => 'The :attribute field must not be greater than :max_width pixels.',
    'between' => 'The :attribute field must be between :min_width and :max_width pixels in width.',
],

However this caused issue with how ValidatesAttributes, FormatsMessages & ReplacesValues classes/trait dynamically look for method names in the validation & replacing of the parameters, so to avoid the scope & size of the PR growing to large, I have kept it simple.

@taylorotwell
Copy link
Member

Has conflicts. Can you also add more description and examples to the PR?

@taylorotwell taylorotwell marked this pull request as draft September 9, 2024 14:07
@CamKem CamKem force-pushed the feat/granular-dimension-messages branch from 92da545 to 5812d03 Compare September 9, 2024 14:08
# Conflicts:
#	src/Illuminate/Validation/Concerns/ValidatesAttributes.php
#	src/Illuminate/Validation/Rules/Dimensions.php
#	tests/Validation/ValidationDimensionsRuleTest.php
@CamKem CamKem marked this pull request as ready for review September 17, 2024 10:51
@CamKem
Copy link
Contributor Author

CamKem commented Sep 22, 2024

Has conflicts. Can you also add more description and examples to the PR?

Description updated to be more thorough & include examples. Let me know if you need any additional info.

@taylorotwell
Copy link
Member

I think I'm going to hold off on this one for now.

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