rainlab / translate-plugin

Enables multi-lingual sites
Other
125 stars 88 forks source link

The plugin does not run validation for translatable fields #450

Closed GinoPane closed 1 year ago

GinoPane commented 5 years ago

While working on GinoPane/oc-blogtaxonomy-plugin#14 I've discovered that the translatable plugin does not actually run any validators registered for translatable fields.

So, for example, if I have validation for some title, it will be validated only for primary language, all other language inputs are ignored.

Are there any existing solutions for this?

w20k commented 5 years ago

Hi, @GinoPane I'm not really proficient with translate plugin, so can't help you with that, but here is a group of PRO Devs who might know and help. Gurus, I summon you - @munxar @mjauvin @osmanzeki πŸ˜„

PS: Joking, Thank you in advance! ;)

osmanzeki commented 5 years ago

I tried it and it does not run the validation rules on the other languages indeed. Don't know why I never noticed this before.

There is a workaround at the moment if you use the model's lifecycle events as described here and run custom validations for each active language for example.

Model events : https://octobercms.com/docs/database/model#events

Custom validation : https://octobercms.com/docs/services/validation

Getting a list of all enabled locales : https://github.com/rainlab/translate-plugin/blob/master/components/AlternateHrefLangElements.php#L25

It is a bit troublesome indeed and should definitely be done by the plugin if possible. What do you guys think? Any idea if this is possible or what the best approach would be? @LukeTowers @munxar @mjauvin

"Naive" example of usage of Model events to work around this issue for now :

<?php

namespace App\YourApp\Models;

use Model;

class Page extends Model
{
    use \October\Rain\Database\Traits\Validation;

    public $implement = [
        'RainLab.Translate.Behaviors.TranslatableModel',
    ];

    public $translatable = [
        // Regular title attribute with a real SQL column associated to it
        'title',

        // Slug
        [
            'slug',
            'index' => true,
        ]
    ];

    public $table = 'your_model';

    public $rules = [
        'title' => 'max:100',
        'slug' => 'required|unique:your_model',
    ];

   function beforeValidate() {
        $this->validateLocalizedContent();
   }

    private function validateLocalizedContent()
    {
        // NOTE: This could be a loop of all langs instead of doing it manually
        $this->validateLocalizedContentByLocale('fr');
        $this->validateLocalizedContentByLocale('en');
    }

    private function validateLocalizedContentByLocale(string $locale)
    {
        // Validate according to custom rules. If possible perhaps use the same rules as the existing model?
        // You could definitely use the " $this->rules " array here to reuse the model's already declared rules if you wish to do so. If you want to validate other langs differently, you can also omit that step and write your own. For example, most of the times in French the texts are longer than in English, we sometimes need to validate a specific locale differently (but very rarely).
    }

}
osmanzeki commented 5 years ago

For reference, I believe the validation magic is happening here:

https://github.com/octobercms/library/blob/8d96a7b239f8b30984ccde8dbfc525558d053197/src/Database/Traits/Validation.php#L162

And more precisely here: https://github.com/octobercms/library/blob/8d96a7b239f8b30984ccde8dbfc525558d053197/src/Database/Traits/Validation.php#L200

Is there a way we could do this in the RainLab.Translate context instead of modifying the core library?

mjauvin commented 5 years ago

@osmanzeki https://github.com/osmanzeki Can you try to monkey patch the Rainlab.Translate plugin to force the bindEvent priority to something HIGHER than the Validation Trait's bootValidation which is set at 500?

You need to change this in classes/TranslatableBehavior.php::__construct()... add a third parameter to the model.saveInternal bindEvent() and use 1000 for example

If I'm right, this should fix the problem, otherwise we'll need to keep investigating.

On Wed, Feb 27, 2019 at 10:04 AM Osman Zeki notifications@github.com wrote:

For reference, I believe the validation magic is happening here:

https://github.com/octobercms/library/blob/8d96a7b239f8b30984ccde8dbfc525558d053197/src/Database/Traits/Validation.php#L162

And more precisely here:

https://github.com/octobercms/library/blob/8d96a7b239f8b30984ccde8dbfc525558d053197/src/Database/Traits/Validation.php#L200

Is there a way we could do this in the RainLab.Translate context instead of modifying the core library?

β€” You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/rainlab/translate-plugin/issues/450#issuecomment-467896317, or mute the thread https://github.com/notifications/unsubscribe-auth/AB65vt_3Vmtb1mMxgvZFanmClMhUVf20ks5vRp5wgaJpZM4bUeOL .

-- Marc

osmanzeki commented 5 years ago

@osmanzeki https://github.com/osmanzeki Can you try to monkey patch the Rainlab.Translate plugin to force the bindEvent priority to something HIGHER than the Validation Trait's bootValidation which is set at 500? You need to change this in classes/TranslatableBehavior.php::__construct()... add a third parameter to the model.saveInternal bindEvent() and use 1000 for example If I'm right, this should fix the problem, otherwise we'll need to keep investigating. … On Wed, Feb 27, 2019 at 10:04 AM Osman Zeki @.***> wrote: For reference, I believe the validation magic is happening here: https://github.com/octobercms/library/blob/8d96a7b239f8b30984ccde8dbfc525558d053197/src/Database/Traits/Validation.php#L162 And more precisely here: https://github.com/octobercms/library/blob/8d96a7b239f8b30984ccde8dbfc525558d053197/src/Database/Traits/Validation.php#L200 Is there a way we could do this in the RainLab.Translate context instead of modifying the core library? β€” You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#450 (comment)>, or mute the thread https://github.com/notifications/unsubscribe-auth/AB65vt_3Vmtb1mMxgvZFanmClMhUVf20ks5vRp5wgaJpZM4bUeOL . -- Marc

Just tried it with 1000 in the bindEvent method and it seems like it is still not validating the other languages, just the main one.

screen shot 2019-02-27 at 10 32 05 am
GinoPane commented 5 years ago

I'd say that the problem is that Validation trait knows nothing about translated attributes:

image

osmanzeki commented 5 years ago

I'd say that the problem is that Validation trait knows nothing about translated attributes:

image

I feel like if possible the logic handling this should run a layer above that code. Meaning that if the right model is provided to the Trait, it will work properly. That said, I'm not sure exactly how that can be done or if it is the right approach.

It seems like since this is a Trait, every instance of $this refers to the original model (which has the TranslatableBehavior on it but is not the translated instance). If there was a way of running this trait on the translated model or the translatable fields, the validation would follow suit I believe.

GinoPane commented 5 years ago

It looks like it is possible to do something by subscribing on beforeValidate, but I haven't ended up with PoC yet.

GinoPane commented 5 years ago

Ok, PoC is here. In beforeValidate we can populate model attributes with fake locale-driven attributes, fill their values from RLTranslate, fill rules and run validation. Here's the raw example:

image

And in afterValidate we will need to unset those attributes before they will be taken to DB with exception (because the column does not exist).

The problem though with unique validation rule, because it should be run against rainlab_translate_attributes and take into account the locale and real field name (not the fake one). So maybe this validation can be implemented in TranslatableBehavior as well (in afterValidate for example).

Seems doable.

osmanzeki commented 5 years ago

If you look at my example, you can see how you can implement those same custom rules without modifying the core RainLab.Translate files. I'm not sure doing these changes in the TranslatableBehaviour class fixes this problem correctly for all possible use cases.

https://github.com/rainlab/translate-plugin/issues/450#issuecomment-467894920

GinoPane commented 5 years ago

@osmanzeki Yes, but I tried to find a generic solution for the plugin itself. It can be provided as an optional Trait maybe that can be included or not (depending on user's needs).

osmanzeki commented 5 years ago

@osmanzeki Yes, but I tried to find a generic solution for the plugin itself. It can be provided as an optional Trait maybe that can be included or not (depending on user's needs).

Ah yes I see, indeed. Will have to wait and see what others who know the plugin better than I do also think about the best approach here too.

mjauvin commented 5 years ago

I think it would be possible to duplicate the rules based on the defined locales and override the Validation Trait's getValidationAttributes() method to return the full list of localized attributes as follow:

function getValidationAttributes()
{  
   $knownLocales = array_keys($this->translatableAttributes)
   $attributes = getAttributes();

   foreach($this->translatable as $attr) {
      foreach ($knownLocales as $locale) {
         $localizedAttr = $attr . '_' . $locale;
         if ($this->rules[$attr]) {
            $attributes[] = $localizedAttr;
            $this->rules[$localizedAttr] = $this->rules[$attr];
         }
      }
   }  
   return $attributes;
}

This can be done in your Model or in TranslatableModel, I guess. Does that make sense?

mjauvin commented 5 years ago

We still need to fetch the translated data somehow, though...

mjauvin commented 5 years ago

Maybe like this:

function getValidationAttributes()
{
        $knownLocales = array_keys($this->translatableAttributes)
        $attributes = getAttributes();

        foreach($this->translatable as $attr) {
                foreach ($knownLocales as $locale) {
                        $localizedAttr = $attr . '_' . $locale;
                        if ($this->rules[$attr]) {
                                $attributes[$localizedAttr] = $this->getAttributeTranslated($attr, $locale);
                                $this->rules[$localizedAttr] = $this->rules[$attr];
                        }
                }
        }
        return $attributes;
}
osmanzeki commented 5 years ago

Yes indeed, the actual fetching of the data is problematic. If there is a way of running this code without it knowing that it is operating on a translated model, it would be better. That way it hooks into the other systems without breaking anything or introducing potential bugs in my opinion.

For example, I believe this works:


$regularModelAttributes = $model->attributes;
// $regularModelAttributes = [ 'title' => "TITLE" ]

$model->translateContext('fr');

$translatedModelAttributes = $model->attributes;
// $translatedModelAttributes = [ 'title' => "TITRE" ]

Meaning that if we can inject the right model in a higher level system and with this kind of logic, the rest should potentially just work fine without modifying anything. What do you guys think?

I'm not 100% sure that the translateContext function works like that tho. Need to have a second opinion.

osmanzeki commented 5 years ago

Yes indeed, the actual fetching of the data is problematic. If there is a way of running this code without it knowing that it is operating on a translated model, it would be better. That way it hooks into the other systems without breaking anything or introducing potential bugs in my opinion.

For example, I believe this works:


$regularModelAttributes = $model->attributes;
// $regularModelAttributes = [ 'title' => "TITLE" ]

$model->translateContext('fr');

$translatedModelAttributes = $model->attributes;
// $translatedModelAttributes = [ 'title' => "TITRE" ]

Meaning that if we can inject the right model in a higher level system and with this kind of logic, the rest should potentially just work fine without modifying anything. What do you guys think?

I'm not 100% sure that the translateContext function works like that tho. Need to have a second opinion.

My bad. I can confirm that this does not work. The attributes array stays the same no matter what language you're in (regardless of translateContext).

osmanzeki commented 5 years ago

@mjauvin Just tried a PoC of your code, it does not seem to work because the validate function is using the rules it is passed from the outside and not the $this->rules array from the $model. I will try to see if there is another way.

This is the code I used for my PoC on your idea :


    // Context: Validator.php in /vendor/october/rain/src/Database/Traits/Validation.php

    /**
     * Returns the model data used for validation.
     * @return array
     */
    protected function getValidationAttributes()
    {
         // Temporary, we should get all all the languages here (except the original one)
        $knownLocales = ['fr']; 

        $attributes = $this->getAttributes();
        $translatedAttributes = $this->getTranslatableAttributes();

        foreach ($translatedAttributes as $attr) {
            foreach ($knownLocales as $locale) {
                $localizedAttr = $attr . '_' . $locale;

                if (array_key_exists($attr, $this->rules)) {
                    $attributes[$localizedAttr] = $this->getAttributeTranslated($attr, $locale);
                    $this->rules[$localizedAttr] = $this->rules[$attr];
                }
            }
        }

        return $attributes;
    }
osmanzeki commented 5 years ago

Actually, we were creating the rules too late for it to work. Your idea might still work:

        /*
         * Perform validation
         */
        $rules = is_null($rules) ? $this->rules : $rules;
        $rules = $this->processValidationRules($rules);
        $success = true;

        if (!empty($rules)) {

            $data = $this->getValidationAttributes(); // Rules are already created above at this point
mjauvin commented 5 years ago

Right. Try creating a rule manually for the translated field(s) in the Model constructor to see if the rest works... we can always insert the rules earlier in a seperate function / event handler.

osmanzeki commented 5 years ago

Sadly, having rules with a language prefix does not work as October actually tries to use those in the database later on in the process. We need to be able to use the translated model as if it was the original model ideally.

screen shot 2019-02-27 at 4 04 57 pm
mjauvin commented 5 years ago

Can you show the code that leads to this error?

osmanzeki commented 5 years ago

Can you show the code that leads to this error?

Here it is: https://gist.github.com/osmanzeki/a929f6f9483ad4f130a1b66cafcc1b19

My changes were made at the following lines:

Replaced the getValidationAttributes implementation with yours (a slight variation of): https://gist.github.com/osmanzeki/a929f6f9483ad4f130a1b66cafcc1b19#file-validation-php-L119

Moved the getValidationAttributes a little higher in the validate method

To make sure that the $rules var is correct: https://gist.github.com/osmanzeki/a929f6f9483ad4f130a1b66cafcc1b19#file-validation-php-L225

mjauvin commented 5 years ago

Nice work! Then it really is not going to work this way, unfortunately...

GinoPane commented 5 years ago

Sadly, having rules with a language prefix does not work as October actually tries to use those in the database later on in the process. We need to be able to use the translated model as if it was the original model ideally.

screen shot 2019-02-27 at 4 04 57 pm

Cmon guys, I've already told that several messages above (and 4 lines PoC there can be used too: https://github.com/rainlab/translate-plugin/issues/450#issuecomment-467966882) - all you need to do is to unset artificial attributes after the validation (in afterValidate callback for example), it will prevent them from going into DB.

Also do not forget about "unique" rule which won't work for user model, because uniqueness in that case must be checked against the table with translations, so it must be implemented somewhere in Translatable plugin itself (because only it knows its own internal DB structure).

There could be pitfalls but the overall approach seems to be valid.

osmanzeki commented 5 years ago

Indeed the unique rule is problematic. If there was a way to do all of this in the RainLab.Translate plugin it would be better indeed. Invisible to the core.

I keep thinking that if the RainLab.Translate could give us a mock instance of a translated model with its attributes and rules in the translated language every else would work and every other possible bug like this one could be covered. That way we have a single code path possible and not a million "ifs" everywhere handling translation edge cases.

osmanzeki commented 5 years ago

In Voyager (another Laravel-based CMS), there is an option to create a "translated" model instance that can be used like the regular model. Perhaps that could be an inspiration. That way all rules and attributes are always usable "normally" no special exceptions for the translated versions of a model.

See here for reference : https://docs.laravelvoyager.com/core-concepts/multilanguage#translate-the-whole-model

mjauvin commented 5 years ago

@osmanzeki isn't it what "$this->model" gives us in classes/TranslatableBehavior.php ?

osmanzeki commented 5 years ago

@mjauvin No it seems like the TranslatableBehavior class is storing any translatable attribute in its own variables and just keep a reference to the original model, untouched.

https://github.com/rainlab/translate-plugin/blob/master/classes/TranslatableBehavior.php#L39

Then another class extends this class and implements abstract methods used for "how" or "where" the data is actually loaded (in this case from the translation tables in the DB).

https://github.com/rainlab/translate-plugin/blob/60e88cb383e8a46c887459c63f87b2f6d50da635/behaviors/TranslatableModel.php#L174

So there is never an option to have a copy of the regular model with its values replaced by translated one like Voyager offers.

mjauvin commented 5 years ago

Oh, I see what you mean now... right.

On Thu, Feb 28, 2019 at 11:10 AM Osman Zeki notifications@github.com wrote:

@mjauvin https://github.com/mjauvin No it seems like the TranslatableBehavior class is storing any translatable attribute in its own variables and just keep a reference to the original model, untouched.

https://github.com/rainlab/translate-plugin/blob/master/classes/TranslatableBehavior.php#L39

Then another class extends this class and implements abstract methods used for "how" or "where" the data is actually loaded (in this case from the translation tables in the DB).

https://github.com/rainlab/translate-plugin/blob/60e88cb383e8a46c887459c63f87b2f6d50da635/behaviors/TranslatableModel.php#L174

So there is never an option to have a copy of the regular model with its values replaced by translated one like Voyager offers.

β€” You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/rainlab/translate-plugin/issues/450#issuecomment-468331958, or mute the thread https://github.com/notifications/unsubscribe-auth/AB65vk8eY0l1hzgutpf_I4uoK51cHYWQks5vR_9mgaJpZM4bUeOL .

-- Marc

lalomolino commented 4 years ago

I am starting my first project in October and I was very excited to see that the CMS had a plugin that allowed to create multilanguage sites in such an easy way but I have been trying for validation for a week now because I have 3 languages ​​and I do not succeed, I have tried several things of which you raised but I have not been able to do it, have you found a definitive solution?

PS: Thank you in advance! ;)

daftspunk commented 1 year ago

I can confirm that the issue has been resolved in OCMS v3 and Translate v2.