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

Fix setting model attribute with setter to use setter #11401

Merged
merged 1 commit into from
Feb 12, 2016
Merged

Fix setting model attribute with setter to use setter #11401

merged 1 commit into from
Feb 12, 2016

Conversation

nsossonko
Copy link
Contributor

This syncs up the behavior with assign() and setting the value directly and also aligns the behavior of getting the property with setting the property. Comes with a test.

@sergeyklay
Copy link
Contributor

Can you please merge your efforts with #11320 into one PR.

Amend CHENGELOG, update tests, explain a bit more what issue can be solved (or what feature can be introduced) with your PR, etc.

Thanks.

@nsossonko
Copy link
Contributor Author

Sorry I didn't even notice that PR. This PR is strictly about looking for a setter when assigning a value directly to a hidden property. That PR solves some other things too.

Basically, Phalcon supports defining getters/setters for model attributes that are called automatically when they are retrieved and/or set. The problem is, when the attribute is set directly it is not called, instead the attribute is assigned whatever value is being passed.

This would fix Issue #11286. Feel free to close if the other PR precedes this one (although feel free to take the test from this one and use in the other one too).

@nsossonko
Copy link
Contributor Author

@sergeyklay what do you think should be done? This change should be completely compatible with 2.0.x and master as well...

@sergeyklay
Copy link
Contributor

Can you please rebase?

@nsossonko
Copy link
Contributor Author

@sergeyklay done (I'm pretty sure). Thanks.

$this->_executeSetGet();
}

/*public function testModelsPostgresql()
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are these commented out?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because this doesn't really touch the database anyhow (it just checks the getters/setters for the model object).

@sergeyklay
Copy link
Contributor

Could you please submit just the commits that belong to the PR?

@nsossonko
Copy link
Contributor Author

@sergeyklay done. I messed up with my git rebasing which caused that issue but should be fully fixed and up to date now.

sergeyklay added a commit that referenced this pull request Feb 12, 2016
Fix setting model attribute with setter to use setter
@sergeyklay sergeyklay merged commit ed2b783 into phalcon:2.1.x Feb 12, 2016
@sergeyklay
Copy link
Contributor

Thanks

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

Successfully merging this pull request may close these issues.

3 participants