Skip to content
This repository has been archived by the owner. It is now read-only.

Add support for Model accessors to be called #257

Merged
merged 4 commits into from Aug 17, 2016

Conversation

@easteregg
Copy link
Contributor

commented Aug 10, 2016

This fixes #256 .

@@ -162,6 +162,10 @@ public function getAttribute($key)
return;
}
if ($this->hasGetMutator($key)) {
return $this->mutateAttribute($key, $this->getTranslation($locale)->$key);

This comment has been minimized.

Copy link
@dimsav

dimsav Aug 10, 2016

Owner

Why do you call call $this->mutateAttribute() instead of $this->getAttributeValue($key)?

This comment has been minimized.

Copy link
@easteregg

easteregg Aug 10, 2016

Author Contributor

Thank you for the reply, @dimsav .

$this->getAttributeValue($key) will check the Model's $attribute array. and does not get the value from the translation. then internally checks if the $key has a mutator, then calls the mutator with the Model's $value (which is not translated.).

So calling $this->getAttributeValue($key) does not mutate translated attributes.

But, you are right, using the $this->getAttributeValue($key) makes more sense, so I will change the code (by checking if $key has a mutator, then push it to $attributes before calling $this->getAttributeValue($key).

@dimsav

This comment has been minimized.

Copy link
Owner

commented Aug 10, 2016

Thanks a lot for this PR! Can you write a test that these changes will fix?

@easteregg

This comment has been minimized.

Copy link
Contributor Author

commented Aug 10, 2016

I will write the test ASAP for this.

easteregg added some commits Aug 10, 2016

Add support for Model accessors to be called after getting the transl…
…ation.

Change the behavior to use Model::getAttributeValue() method.

Add a test for mutating translated attribute

Fixing schema rollback

add missing namespace
@easteregg

This comment has been minimized.

Copy link
Contributor Author

commented Aug 17, 2016

Ok, the tests are passing. Do you want me to squash commits?

Oh, and BTW, I've seen the tests are extremely slow, take a look at this branch which I tried to run the tests as in memory sqlite, and it is way faster than mysql.

@dimsav

This comment has been minimized.

Copy link
Owner

commented Aug 17, 2016

Thanks @easteregg!

Indeed the tests are slow. From my experience running tests in sqlite is a bad practice because some mysql errors are not shown.

@dimsav dimsav merged commit 3e73600 into dimsav:master Aug 17, 2016

2 checks passed

continuous-integration/styleci/pr The StyleCI analysis has passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@dimsav

This comment has been minimized.

Copy link
Owner

commented Aug 17, 2016

The changes were just released. There are interesting techniques we could use to speedup mysql in tests. We can do this at a later moment.

@easteregg

This comment has been minimized.

Copy link
Contributor Author

commented Aug 17, 2016

Thank you for your answer. I guess this issue is closed now.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
2 participants
You can’t perform that action at this time.