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

[1.x] Implement Support for Translatable Validation Attribute Errors #4070

Open
wants to merge 4 commits into
base: 1.x
Choose a base branch
from

Conversation

DavideIadeluca
Copy link
Contributor

@DavideIadeluca DavideIadeluca commented Oct 12, 2024

Fixes #0000

Changes proposed in this pull request:

  • Implement support for translating attributes of backend validation errors.
  • Add English translations for new attributes

Reviewers should focus on:

Screenshot

QA

  1. Install a language pack such as flarum-lang/german or flarum-lang/dutch
  2. Switch the Forum UI to another language (not english)
  3. Try to submit a new discussions with no title or content. You should now see the translated version of title or content

Necessity

  • Has the problem that is being solved here been clearly explained?
  • If applicable, have various options for solving this problem been considered?
  • For core PRs, does this need to be in core, or could it be in an extension?
  • Are we willing to maintain this for years / potentially forever?

Confirmed

  • Frontend changes: tested on a local Flarum installation.
  • Backend changes: tests are green (run composer test).
  • Core developer confirmed locally this works as intended.
  • Tests have been added, or are not appropriate here.

Required changes:

  • Related documentation PR: (Remove if irrelevant)
  • Related core extension PRs: (Remove if irrelevant)

@DavideIadeluca DavideIadeluca marked this pull request as ready for review October 12, 2024 18:49
@DavideIadeluca DavideIadeluca requested a review from a team as a code owner October 12, 2024 18:49
@SychO9
Copy link
Member

SychO9 commented Oct 12, 2024

It should be possible to edit AbstractValidator and manipulate the messages to dynamically check for the existence of a validation attribute translation. Mainly, the result of getMessages.

@SychO9
Copy link
Member

SychO9 commented Oct 12, 2024

The only concern is potential conflicts in attribute names between extensions. So might be better to put extension validation under the extension namespace.

Copy link
Member

@imorland imorland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As @SychO9 suggests, we run the possibility of conflicts here if extensions register the same attribute names, but want a different translation.

Let's scope everything under the extension name(s) please

@DavideIadeluca
Copy link
Contributor Author

@imorland I moved the validation translations to the extension namespaces

It should be possible to edit AbstractValidator and manipulate the messages to dynamically check for the existence of a validation attribute translation. Mainly, the result of getMessages.

@SychO9 Can you make an example of what you were thinking? Now that some validation strings are in the extension namespaces, I feel like it would get messy if the AbstractValidator has to support both the structure of core and extensions but maybe I'm missing something here

@SychO9
Copy link
Member

SychO9 commented Oct 19, 2024

here is a proof of concept of how you can obtain the extension ID related to the validator:

// this would be nice in a trait, used by the abstract validator in this case
protected function getClassExtensionId(): ?string
{
    $extensions = resolve(ExtensionManager::class);

    return $extensions->getExtensions()
                ->mapWithKeys(function (\Flarum\Extension\Extension $extension) {
                    return [$extension->getId() => $extension->getNamespace()];
                })
                ->where(function ($namespace) use ($class) {
                    return $namespace && str_starts_with(static::class, $namespace);
                })
                ->keys()
                ->first();
}

you would have to add this to Extension.php

    public function getNamespace(): ?string
    {
        return Collection::make($this->composerJsonAttribute('autoload.psr-4'))
            ->filter(function ($source) {
                return $source === 'src/';
            })
            ->keys()
            ->first();
    }

once you have that, you could theoretically use it in the getMessages method to automatically point to $extId.validation.attributes.$attribute or validation.attributes.$attribute if core (extId null)

also imo it's not necessary to replace . with _ for attribute locale key.

@imorland imorland changed the title Implement Support for Translatable Validation Attribute Errors [1.x] Implement Support for Translatable Validation Attribute Errors Oct 21, 2024
@DavideIadeluca DavideIadeluca marked this pull request as draft October 26, 2024 16:29
@imorland
Copy link
Member

imorland commented Nov 9, 2024

I'll tentatively add this to the 1.8.9 milestone, if you have the chance to complete it before the due date, then I'm happy to include it in that release @DavideIadeluca

@imorland imorland added this to the 1.8.9 milestone Nov 9, 2024
@DavideIadeluca
Copy link
Contributor Author

DavideIadeluca commented Nov 14, 2024

@imorland I've now refactored the implementation per Samis suggestion. Three cases that are knowingly missing here is for example the validation logic in SaveTagsToDatabase. The SaveTagsToDatabase class creates it's own validator, which then doesn't use the new logic.

protected function validateTagCount($type, $count)
{
$min = $this->settings->get('flarum-tags.min_'.$type.'_tags');
$max = $this->settings->get('flarum-tags.max_'.$type.'_tags');
$key = 'tag_count_'.$type;
$validator = $this->validator->make(
[$key => $count],
[$key => ['numeric', $min === $max ? "size:$min" : "between:$min,$max"]]
);
if ($validator->fails()) {
throw new ValidationException([], ['tags' => $validator->getMessageBag()->first($key)]);
}
}

There are two other examples where a new validator is created instead of the AbstractValidator extended:

  • SettingsValidator
  • RegisterUserHandler

Should refactoring of the non-standard validation logic be done in this PR or separately (or not at all)?

Also, would it make sense to add tests for this?

@DavideIadeluca DavideIadeluca marked this pull request as ready for review November 14, 2024 15:14
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.

3 participants