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

Cast attributes are saved to db and to 'rainlab_translate_attributes'… #322

Conversation

yarbala
Copy link

@yarbala yarbala commented Nov 29, 2017

… but have empty value for default language in editor

Model.php:

public $translatable = ['title', 'fields[test11]', 'fields[test12]', 'fields[test21]'];
protected $casts = [
'fields' => 'array'
];
fields.yml:

    'fields[test11]':
        label: Test11
        type: text
        tab: 'Content'
    'fields[test12]':
        label: Test12
        type: text
        tab: 'Content'

After saving data I have proper values in DB. I open this form for editing and switch to not default language, then back to default - and field becomes empty.

I'm using latest october version. For other field types like Markdown it works same way.

… but have empty value for default language in editor

Model.php:

public $translatable = ['title', 'fields[test11]', 'fields[test12]', 'fields[test21]'];
protected $casts = [
        'fields' => 'array'
    ];
fields.yml:

        'fields[test11]':
            label: Test11
            type: text
            tab: 'Content'
        'fields[test12]':
            label: Test12
            type: text
            tab: 'Content'
After saving data I have proper values in DB. I open this form for editing and switch to not default language, then back to default - and field becomes empty.

I'm using latest october version. For other field types like Markdown it works same way.
@iboved
Copy link

iboved commented May 18, 2018

Why Pull request has not yet merged? I have the same problem.

@LukeTowers
Copy link
Contributor

@iboved because I haven't had time to review and test it thoroughly yet. If it's something you need then please test it locally and try as many different ways to break it as possible so that we can merge a solidly tested fix.

@amdad
Copy link

amdad commented Nov 2, 2018

@LukeTowers I can confirm the issue still exist. It's almost a year old,
This PR solve the issue, so if you don't have better option, please accept this and it can be kept as pending (bleeding edge) update to test. But without it, we have broken most fundamental function on fields. It's critical issue.

@LukeTowers
Copy link
Contributor

@yarbala @iboved @amdad

I haven't merged this in yet because I don't understand what this fix is doing and I don't have time to become acquainted enough with the issue myself to figure out how this fix works. If someone can break it down for me line by line as to how this fix works then I'd be happy to merge it in.

@osmanzeki
Copy link
Contributor

Hi everyone,

I've implemented a proof-of-concept with some explanations in the following repo for the fix in this PR:
https://github.com/osmanzeki/octobercms-jsonable-fix

You should be able to run this pretty easily by reading my README.MD file in there and by importing my database dump. I will try to capture a few videos or gifs showing the bugs too to make it even clearer in the future.

Cheers,
Oz

@LukeTowers @yarbala @iboved @amdad

@osmanzeki
Copy link
Contributor

osmanzeki commented Nov 13, 2018

I noticed by testing the stuff I did in the themes (I'm using the translated jsonable values in there as testing grounds) that this approach seems to break a few things. When a field is in a jsonable array but is not in the translatable, it seems like you can no longer get the value when fetching the "translated" values in the same jsonable column.

See the meta[image] value in my examples here:

https://github.com/osmanzeki/octobercms-jsonable-fix/blob/master/themes/demo/pages/home.htm#L22

and

https://github.com/osmanzeki/octobercms-jsonable-fix/blob/master/themes/demo/pages/home.htm#L51

I can fetch the translated meta[title] and meta[description] values in all languages but I can only get the meta[image] value in English (the default language). Seems like the change is breaking the behavior of the hasTranslation method in /plugins/rainlab/translate/classes/TranslatableBehavior.php.

@osmanzeki
Copy link
Contributor

osmanzeki commented Nov 13, 2018

I am starting to think that we should not be doing the json_decode in the getAttributeFromData method as this PR and my test-repo does but perhaps we should be doing decoding earlier in the lifecycle of these calls and thus giving the data "already decoded" to the method instead if possible.

Let me know if you guys have some input on this.

@mjauvin
Copy link
Contributor

mjauvin commented May 9, 2019

Closing as it has been over a month since any activity on this occurred and we are trying to figure out what issues are still relevant. If this is still something that you would like to see through to fruition please respond and we can get the ball rolling.

@mjauvin mjauvin closed this May 9, 2019
@mjauvin mjauvin changed the title Cast attributes are saved to db and to 'rainlab_translate_attributes'… Cast attributes are saved to db and to 'rainlab_translate_attributes'… Jun 2, 2019
@mjauvin mjauvin changed the title Cast attributes are saved to db and to 'rainlab_translate_attributes'… Cast attributes are saved to db and to 'rainlab_translate_attributes'… Jun 2, 2019
@mjauvin mjauvin self-assigned this Jun 2, 2019
@mjauvin mjauvin reopened this Jun 2, 2019
@mjauvin
Copy link
Contributor

mjauvin commented Jun 2, 2019

@osmanzeki first of all, thanks for the very nice and complete description of the issue.

Would it be possible to do the json decoding here:

https://github.com/osmanzeki/octobercms-jsonable-fix/blob/master/plugins/rainlab/translate/classes/TranslatableBehavior.php#L160

With the proper conditions added (like check if the part without the index is in $jsonable or $casts property? i.e. meta[title] -> meta... is meta in $jsonable or $casts property, do the decoding)

@mjauvin
Copy link
Contributor

mjauvin commented Jun 2, 2019

Something like this maybe (not tested):

diff --git a/classes/TranslatableBehavior.php b/classes/TranslatableBehavior.php
index 6cfc3fa..848b221 100644
--- a/classes/TranslatableBehavior.php
+++ b/classes/TranslatableBehavior.php
@@ -160,7 +160,7 @@ abstract class TranslatableBehavior extends ExtensionBase
         if (
             is_string($result) &&
             method_exists($this->model, 'isJsonable') &&
-            $this->model->isJsonable($key)
+            ($this->model->isJsonable($key) || $this->model->isJsonable(HtmlHelper::nameToArray($key)[0]))
         ) {
             $result = json_decode($result, true);
         }

@osmanzeki
Copy link
Contributor

Hey Marc,

Sorry for the late reply, I have been out of the country for work for the last month or so, just came back to my senses here. :)

I would need to immerse myself back into the problem to be sure for your question. I've added a reminder in my calendar to try to do that this week or the next one.

That said, I think the problem here was that there was a few missing links between all the systems that the translation module interacts with and thus that trying to do the json decoding too late down the function execution order was leading to problems. I'm not sure if this location is early enough to alleviate the issues down the line.

Especially when something is translatable but not a string/text like the image field, file field, another json like the Repeaters, etc.

I think with more test coverage on the rainlab translate plugin we could be better suited to identify where the chain seems to break sometimes.

@mjauvin
Copy link
Contributor

mjauvin commented Dec 18, 2019

Closing as it has been over a month since any activity on this occurred and we are trying to figure out what issues are still relevant. If this is still something that you would like to see through to fruition please respond and we can get the ball rolling.

@mjauvin
Copy link
Contributor

mjauvin commented Feb 16, 2020

fix for #303

@mjauvin
Copy link
Contributor

mjauvin commented Mar 15, 2020

Closed in favor of simpler fix in #556

@mjauvin mjauvin closed this Mar 15, 2020
@mjauvin mjauvin removed this from the 1.7.2 milestone Jun 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants