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

added "skipOnError" ability & minor changes #54

Merged
merged 18 commits into from Feb 28, 2020
Merged

Conversation

@thenotsoft
Copy link
Contributor

thenotsoft commented Feb 27, 2020

Q A
Is bugfix?
New feature? ✔️
Breaks BC?
Tests pass? ✔️
Fixed issues #29
@samdark samdark requested review from FabrizioCaldarelli and yiisoft/reviewers and removed request for FabrizioCaldarelli Feb 27, 2020
src/Rule.php Outdated Show resolved Hide resolved
src/ResultSet.php Outdated Show resolved Hide resolved
{
$this->translator = $translator;
return $this;
$new = clone $this;

This comment has been minimized.

Copy link
@kamarton

kamarton Feb 28, 2020

Contributor

Do you really need to create a new copy? I don't think there is a use-case. If your goal is cascading, you should:

public function translator(TranslatorInterface $translator): self 
{
  $this->translator = $translator;
  return $this;
}

$validator->translator(...)->translatorDomain(...)->translationLocale(...)->...

This comment has been minimized.

Copy link
@samdark

samdark Feb 28, 2020

Member

@kamarton cloning is cheap. I don't think there's anything bad in making object immutable if it's not meant to have state.

@@ -73,6 +82,13 @@ public function translateMessage(string $message, array $arguments = []): string
);
}

public function skipOnError(bool $value): self

This comment has been minimized.

Copy link
@kamarton

kamarton Feb 28, 2020

Contributor

If you have a new instance: withSkipOnError(), but I suggest that you do not have a new instance here:

public function skipOnError(bool $value): self
{
        $this->skipOnError = $value;
        return $this;
}
foreach ($this->attributeRules as $attribute => $rules) {
$results->addResult(
$attribute,
$rules->validate($dataSet->getAttributeValue($attribute))
$rules->validate($dataSet->getAttributeValue($attribute), $dataSet)

This comment has been minimized.

Copy link
@samdark

samdark Feb 28, 2020

Member

This is not related to skip on error but is important bugfix.

{
$this->translator = $translator;
return $this;
$new = clone $this;

This comment has been minimized.

Copy link
@samdark

samdark Feb 28, 2020

Member

@kamarton cloning is cheap. I don't think there's anything bad in making object immutable if it's not meant to have state.

@samdark samdark merged commit 00c76d8 into yiisoft:master Feb 28, 2020
2 checks passed
2 checks passed
Scrutinizer Analysis: 5 updated code elements – Tests: passed
Details
Travis CI - Pull Request Build Passed
Details
@samdark

This comment has been minimized.

Copy link
Member

samdark commented Feb 28, 2020

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.