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

Set the raw data array without any mutations for the Entity #2011

Merged
merged 2 commits into from
May 20, 2019
Merged

Set the raw data array without any mutations for the Entity #2011

merged 2 commits into from
May 20, 2019

Conversation

iRedds
Copy link
Collaborator

@iRedds iRedds commented May 16, 2019

Description
When we create, for example, the password hashing method Entity::setPassword(), this method is also called when the value from the database is filled.
As a result, we get a hash of the hash in $entity->password

This PR sets the value of the $attribute property to the unchanged values from the database.

Checklist:

  • Securely signed commits
  • Component(s) with PHPdocs
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

Signed-off-by: Andrey Pyzhikov <5071@mail.ru>
Copy link
Member

@lonnieezell lonnieezell left a comment

Choose a reason for hiding this comment

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

Good catch. But I think it can be done simpler by calling setAttributes/setRawArray instead of fill in the Entity constructor. That should eliminate all of the other checks and fills in the database layer.

* @param array $data
* @return $this
*/
public function setRawArray(array $data)
Copy link
Member

Choose a reason for hiding this comment

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

Rename to setAttributes please.

Copy link
Collaborator Author

@iRedds iRedds May 18, 2019

Choose a reason for hiding this comment

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

What checks do you mean?
And what do you think about changing the class name from Entity to AbstractEntity?

Copy link
Member

Choose a reason for hiding this comment

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

Not a fan of the name change - it's not an abstract class. Even then not sure i'd like it.

Was referring to the checks in the database layer that you've added where it checks if it's an Entity and then uses setRawArray. If we move that to the Entity constructor I don't think any of those checks are needed since things will be set to their raw values the way the database layer currently works.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Then we say goodbye to the stdClass class and the custom class.
Because stdClass has no constructor, and the custom class may not have a constructor.

Copy link
Member

Choose a reason for hiding this comment

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

Nothing's changing there. It works currently. The database engine itself handles that in $this->resultID->fetch_object($className);.

I'm not saying you should remove existing functionality - but remove the portions you added in this PR. They all look something like:

if (is_subclass_of($className, Entity::class))
		{
			$data = $this->fetchAssoc();
			return empty($data) ? false : (new $className())->setRawArray($data);
		}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm confused.
If I remove this code

if (is_subclass_of($className, Entity::class))
{
    return empty($data = $this->fetchAssoc()) ? false : new $className($data);
}

then this PR does not make sense.

Copy link
Member

Choose a reason for hiding this comment

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

Actually - I might be dense here. I was thinking that it would set through the constructor, but it doesn't. It tries to set the value directly on the object. It's late and I'm tired and distracted with other problems. Sorry - what you've done works fine. Just rename that method, please.

Signed-off-by: Andrey Pyzhikov <5071@mail.ru>
@lonnieezell lonnieezell merged commit fa575f9 into codeigniter4:develop May 20, 2019
@iRedds iRedds deleted the feature/entity-raw-data branch May 20, 2019 05:12
@neznaika0
Copy link
Contributor

Any other reason besides setting a password?
I extended the Entity class by adding private properties. abandoned the magic of setters/getters - created get* set*
Now setAttributes cannot call set*
So fill() is the better solution.
Regarding the password hash - this operation is done in the beforeInsert Model or by setting the hash yourself

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