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

Make Model::{data,dirty,scope} props private #864

Merged
merged 2 commits into from
Apr 26, 2021
Merged

Conversation

mvorisek
Copy link
Member

@mvorisek mvorisek commented Apr 26, 2021

for #862, we must have full control over these properties

@mvorisek mvorisek changed the title Update visibility of Model::{data, dirty, scope} props to private Update visibility of Model::{data,dirty,scope} props to private Apr 26, 2021
@mvorisek mvorisek force-pushed the model_refgetter_data branch from 627db51 to 923dad6 Compare April 26, 2021 16:25
@mvorisek mvorisek changed the title Update visibility of Model::{data,dirty,scope} props to private Make Model::{data,dirty,scope} props private Apr 26, 2021
@mvorisek mvorisek force-pushed the model_refgetter_data branch 2 times, most recently from 7b20257 to 69257d5 Compare April 26, 2021 16:27
@@ -442,6 +442,16 @@ private function initEntityHooks(): void
$this->onHookShort(self::HOOK_AFTER_SAVE, $checkFx, [], -10);
}

public function &getDataRef(): array
Copy link
Member

Choose a reason for hiding this comment

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

Don't we need to describe (comment) this and below method? It's quite straightforward, but still...

Copy link
Member Author

@mvorisek mvorisek Apr 26, 2021

Choose a reason for hiding this comment

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

I will add @internal phpdoc, method is to easy migration for #862 before we remove this internal state method completely.

@@ -620,7 +630,8 @@ public function isDirty(string $field): bool
{
$this->checkOnlyFieldsField($field);

if (array_key_exists($field, $this->dirty)) {
$dirtyRef = &$this->getDirtyRef();
if (array_key_exists($field, $dirtyRef)) {
Copy link
Member

Choose a reason for hiding this comment

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

Here we can simplify to return array_key_exists($field, $dirtyRef); don't we?

Copy link
Member

Choose a reason for hiding this comment

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

And this method looks exaactly the same as _isset($field) method below, so to avoid code duplication we could just use return $this->_isset($field); here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch, should be done later with another PR, probably after #862. The issue is, at least from my POV, why should be use dirty array for _isset instead of standard model data?

@mvorisek mvorisek force-pushed the model_refgetter_data branch from 69257d5 to 5fc8813 Compare April 26, 2021 20:16
@mvorisek mvorisek force-pushed the model_refgetter_data branch from 5fc8813 to d927ef5 Compare April 26, 2021 20:16
@mvorisek mvorisek merged commit b9268f4 into develop Apr 26, 2021
@mvorisek mvorisek deleted the model_refgetter_data branch April 26, 2021 20:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants