Skip to content
This repository has been archived by the owner on Jun 18, 2019. It is now read-only.

added getTranslationsArray() method #347

Merged
merged 5 commits into from
Jun 6, 2017
Merged

added getTranslationsArray() method #347

merged 5 commits into from
Jun 6, 2017

Conversation

askello
Copy link
Contributor

@askello askello commented May 13, 2017

According to issue #273, it would be great to have a way to retrieve translations in the same structure such as for saving (like fill() method). For example I have a view with a form for updating some of my translatable records. My form fields named en[title], en[description], es[title] and so on. It is easy to store these data (via create() or fill() methods), and they can be easily passed to the view via $input->old() method. But there is no way to retrive and pass translations data to the view directly from translatable model, because their stucture is different. What do you think about it?

Copy link
Collaborator

@Gummibeer Gummibeer left a comment

Choose a reason for hiding this comment

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

The method itself looks good and the feature introduced by it also.
But the unittest is no unittest for me - you test your logic with your copied logic. The assertEquals comparison array should be hard in the test to ensure that the method returns what we expect.
Or you split it in some smaller tests - test existing keys, test every single value etc.

@unitedworx
Copy link
Contributor

it would be nice if this was also added in 6.* version which is the one compatible with 5.1 LTS

@dimsav dimsav merged commit ff2a07c into dimsav:master Jun 6, 2017
@unitedworx
Copy link
Contributor

@askello i think the test for this method is flawed! i think you should use the same array to fill the model and then test if it equals with the array returned from getTranslationsArray() method! IMHO

@dimsav
Copy link
Owner

dimsav commented Jun 8, 2017

@unitedworx thanks for your feedback. I've fixed the test after merging.

@unitedworx
Copy link
Contributor

wow! am impressed :) wonderful work Dimitris

@askello
Copy link
Contributor Author

askello commented Jun 8, 2017

@unitedworx, you right. @dimsav, thank you for operativity!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants