Skip to content

[10.x] Remove option from make:rule re-added by mistake #44543

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

Merged
merged 1 commit into from
Oct 12, 2022
Merged

[10.x] Remove option from make:rule re-added by mistake #44543

merged 1 commit into from
Oct 12, 2022

Conversation

rodrigopedra
Copy link
Contributor

PR #44016 re-added the --invokable option to the MakeRuleCommand after it was removed by PR #43868

As Invokable rules will be the default in 10.x, this PR removes this option again.

PR #44016 re-added the `--invokable` option to the `MakeRuleCommand` after it was removed by PR #43868

As Invokable rules will be the default in 10.x, this PR removes this option again.
@rodrigopedra
Copy link
Contributor Author

rodrigopedra commented Oct 11, 2022

By the way, would a PR allowing current/regular class rule creation be considered?

I have many rules where I use the translator within the message() method, and when having multiple "failure" points (think of early returns for multiple assessment), repeating the failure message + translation boilerplate when it has placeholders is cumbersome.

I think everyone will use the newer invokable design more, but I guess keeping the old-school style as an easy to create alternative presents no harm.

If this would be considered, I can send a PR.

EDIT: Reading the docs, I see translator has built-in support. Still the many early returns for multiple assessments stands.

As an example I have a custom rule to validate some allowed file formats like below, where to me seems simpler to keep the current style -- maybe I am biased by habit.

public function passes($attribute, $value): bool
{
    if ($this->validateMimes($attribute, $value, $this->extensions)) {
        return true;
    }

    if ($this->validateMimetypes($attribute, $value, $this->mimetypes->all())) {
        return true;
    }

    if ($this->isInvalidFile($value)) {
        return false;
    }

    if ($this->fileHasExpectedMimeType($value)) {
        return true;
    }

    if ($this->isOffice2007File($value)) {
        return true;
    }

    if ($this->isWindowsMediaAudioFile($value)) {
        return true;
    }

    if ($this->expectsTextFile() && $this->clientSaysFileIsTextFile($value)) {
        return ! $this->isBinaryFile($value);
    }

    return false;
}

public function message(): string
{
    return trans('invalid.file', ['extensions' => \implode(', .', $this->extensions)]);
}

For context: most if checks were added after particular valid files -- sent by users -- failed previous checks. This is why some might seem redundant.

@rodrigopedra
Copy link
Contributor Author

Don´t know why tests are failing. This PR just removes a line for an unused command option.

@taylorotwell taylorotwell merged commit 3f9345d into laravel:master Oct 12, 2022
@rodrigopedra rodrigopedra deleted the patch-1 branch October 12, 2022 18:54
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