-
Notifications
You must be signed in to change notification settings - Fork 7
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
Extended validators to allow array values, dependent field validations and added new rules #22
base: master
Are you sure you want to change the base?
Conversation
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
Hi, @hoshomoh, can you help me with this. Checks still haven't completed it seems. |
This is quite the change. I will take some time this coming weekend to review. Great initiative. |
…work for both strings and numbers.
… numbers since it doesnt work as expected, would require dependence on all available rules and value type.
Made a few changes recently
|
* parsed in the same order and format as allowedParameters(). | ||
* This will aid in mapping our parameters to their placeholders. | ||
*/ | ||
public function parseParameterValues(array $parameters): array; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is only useful when there a single parameter is allowed. Instead of doing this, I would rather update
CSVUtils/src/Helpers/FormatsMessages.php
Line 101 in 34afb02
protected function replaceParameterPlaceholder( |
protected function replaceParameterPlaceholder(
string $message,
array $allowedParameters,
array $parameters
): string {
$hasMultipleAllowedParameter = count($allowedParameters) > 1;
$search = $hasMultipleAllowedParameter ? $allowedParameters : $allowedParameters[0];
$replace = $hasMultipleAllowedParameter ? $parameters : implode(',', $parameters);
return str_replace($search, $replace, $message);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this works as well. I just wanted to keep it open in case someone needed multiple parameters.
@@ -88,6 +89,14 @@ protected function makeReplacements( | |||
); | |||
} | |||
|
|||
if ($rule instanceof ArrayParameterizedRuleInterface) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my comment above
* parsed in the same order and format as allowedParameters(). | ||
* This will aid in mapping our parameters to their placeholders. | ||
*/ | ||
public function parseParameterValues(array $parameters): array |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my comment above
* parsed in the same order and format as allowedParameters(). | ||
* This will aid in mapping our parameters to their placeholders. | ||
*/ | ||
public function parseParameterValues(array $parameters): array |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure why this was needed at all. Maybe I am not seeing something. My guess is this will be used like ["requiredIf:field,value"]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, for my use case I had a couple of fields which had to be made required in case some target column had one of the values specified in :other_values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. It seems we can't run away from supporting array parameters. Let's do the then. I will make a fix to support array parameters like so, all rules can accept parameters in one of the following ways
["requiredIf:field,value"]
["requiredIf" => ["field" => "value"]
["requiredIf" => ["field1" => "value1", "field2" => "value2"]
["requiredIf" => ["field1" => ["value1", "value2"]]
What do you think? I will try to push this over the weekend.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good.
@@ -357,6 +370,13 @@ protected function passesParameterCheck($rule, array $parameters): bool | |||
return $parameterCount === $ruleParameterCount; | |||
} | |||
|
|||
if ($rule instanceof ArrayParameterizedRuleInterface) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This no longer needed
Hi,
I am planning to use the library on one of my projects. Thought I'd share some changes here as well to help out the community.
in
andrequired_if
using the above interfacepasses()
function now receives the current validation row which allows for dependent field validation checksrequired_if
which is based on dependent field validationrequired
rule, we can easily ignore empty cells unless required by user explicitlyalpha
,alpha_num
,in
,integer
,numeric
andurl
will now return true for null / empty string valuesLet me know if any change is required.