Use laravel validator to replace avatar validation error params

[SychO9]

Fixes #2933

Changes proposed in this pull request:
When we switched to intl icu message format, the translation parameters
have become required to be in the format {param}, so we can no longer
use the translator here.

This PR switches to consistently rely on the laravel validator to make
the replacements, just like with all other validation errors in the rest
of the application.

Reviewers should focus on:
Is this the best way to go about this ?

Confirmed

• Frontend changes: tested on a local Flarum installation.
• Backend changes: tests are green (run composer test).

[luceos]

Why don't we change the format on the validation.yml strings instead?

[SychO9]

The strings in validation.yml are not specific to the avatar validation which uses the translator, those same validation strings are/will be used by extension directly without custom handling like in the avatar validator, in which case the laravel validator will be responsible for rendering the validation error messages, and it requires the laravel format of :param.

Hence why I opted to just rely on the laravel validator as well, just like we do by default, because we're now in a situation where the translator requires {param} and the laravel validator :param.

[luceos]

I don't know, it still feels like a lot of fuss to get something working. Can't we override the translator instance in the validator to use the ICU one? It's a long shot, but it would result in much cleaner translation files. Now we have to explain why one file behaves differently than the rest.

[SychO9]

I don't know, it still feels like a lot of fuss to get something working. Can't we override the translator instance in the validator to use the ICU one? It's a long shot, but it would result in much cleaner translation files. Now we have to explain why one file behaves differently than the rest.

The laravel validator already uses the flarum translator
https://github.com/flarum/core/blob/e92c267cdec46266b633f71c2f41040731cdaf39/src/Locale/LocaleServiceProvider.php#L56

But it doesn't use the translator to replace the parameters, it uses it just to get the raw message string, then it makes the replacements on its own, hence the makeReplacements method on the validator.

We could also just do manual replacement here instead of relying on the laravel validator for that, I dislike recreating existing logic, but since we're only using three validation strings here, they would all be straightforward replacements. That would be more simple and less of a hustle, and fine for this particular spot, the avatar validator is the only one that does this custom validation anyway.

[askvortsov1]

Is there no way that we could redo this to avoid custom validation instead? Then we wouldn't have a special case. Could we first extract the attributes we need, then run those through the regular validator?

[SychO9]

Is there no way that we could redo this to avoid custom validation instead? Then we wouldn't have a special case. Could we first extract the attributes we need, then run those through the regular validator?

I haven't deeply looked into it yet, but that doesn't seem to be an option by looking at the relevant commit that introduced the custom avatar validation 1cbb2a3#diff-f91b182a730d6646f10d001c0afd69d4042b5eedf5a1c0355e28452855263a18

[askvortsov1]

Is there no way that we could redo this to avoid custom validation instead? Then we wouldn't have a special case. Could we first extract the attributes we need, then run those through the regular validator?

I haven't deeply looked into it yet, but that doesn't seem to be an option by looking at the relevant commit that introduced the custom avatar validation 1cbb2a3#diff-f91b182a730d6646f10d001c0afd69d4042b5eedf5a1c0355e28452855263a18

Instead of trying to run the file directly / copied through the validator, we could preprocess the avatar to extract relevant fields: $guessedExtension, the size, etc. Those could then be validated appropriately.

[SychO9]

Trying to get back to this

Instead of trying to run the file directly / copied through the validator, we could preprocess the avatar to extract relevant fields: $guessedExtension, the size, etc. Those could then be validated appropriately.

that doesn't solve the problem actually though, if we extract those values and run a validator on them instead then the validation error messages generated by the laravel validator will not be descriptive of what's actually going on.

$extractedValues = [
    'status' => $file->getError() === UPLOAD_ERR_OK,
    'size' => $file->getSize(),
    'mimetype' => $this->getMimeType($file),
];

$validator->validate($extractedValues, [
    'status' => 'accepted',
    'size' => 'lte:2048',
    'mimetype' => Rule::in($this->getAllowedTypes()),
]);

the error messages would look something like:

The status must be accepted.
The size must be less than or equal 2048.
The mimetype must be one of: jpg, png, bmp, gif.

Which for some works but then not really.

That said looking at this again, I don't like using the laravel validator to make the replacements either, so we could always

We could also just do manual replacement here instead of relying on the laravel validator for that, I dislike recreating existing logic, but since we're only using three validation strings here, they would all be straightforward replacements. That would be more simple and less of a hustle, and fine for this particular spot, the avatar validator is the only one that does this custom validation anyway.

[askvortsov1]

I understand what you mean now, this looks good to me. One thing though: it would be nice to add a brief comment in the code explaining why we've made this change