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 fill() method in Entity class #3377

Merged
merged 2 commits into from
Jul 22, 2020

Conversation

michalsn
Copy link
Member

Description
When we declare class properties for Entity class then __set() method won't be called.

__set() is run when writing data to inaccessible (protected or private) or non-existing properties

See: #3368

Checklist:

  • Securely signed commits
  • Conforms to style guide

@natanfelles
Copy link
Contributor

I confirm that this resolved issue #3368 . Thank you!

@MGatner
Copy link
Member

MGatner commented Jul 22, 2020

This is a breaking change and IMO it is more of a philosophical change than a bug fix. There are legit reasons a developer may want to declare properties and have them filled but not included in toArray, etc. @natanfelles' issue seems to have been in misunderstanding how Entity properties are declared - you should add them as fields in $attributes rather than class properties.

@michalsn
Copy link
Member Author

@MGatner I can't agree with you as we should take a look at how this method looked like before: https://github.com/codeigniter4/CodeIgniter4/pull/2968/files

From my perspective we simply forgot that parameters may be declared in the class and then __set() won't be triggered.

@MGatner
Copy link
Member

MGatner commented Jul 22, 2020

I think you're right and that was an unintentional BC, but I don't know if it merits another BC to "fix" it.

My personal opinion is I think this is a good change. As is, if someone had a field named "dates" and tried to fill it into an Entity it would really screw the class: $entity = new Entity(['dates' => 2674862738]). This would overwrite the date property, so maybe that constitutes a bug and is worth another BC? Let's keep this discussion going.

@MGatner
Copy link
Member

MGatner commented Jul 22, 2020

Yeah the more I think about this the more I think you're right. We will need to note this as a minor breaking change - unlikely that many people will be affected but it could still mess something up. Can we start a 4.0.5 changelog with this PR and include a note on this interaction? Also I'd like to be sure @lonnieezell sees this.

@lonnieezell
Copy link
Member

I agree this should be fixed. The original intent was that you do exactly what @natanfelles did in his example. so, yes, make a note of it in the the Upgrade Guide that we should start maintaining and I think it's good. :)

@lonnieezell
Copy link
Member

After reading back through the original post, I need to clarify. You should be able to do this:

$user = new User([
  'email' => '[email protected]',
  'name' => 'Joe Smith'
]);
$users = new UserModel();
$users->save($user);

Double-checking against the Model and the change is good.

@michalsn
Copy link
Member Author

I've added info to the changelog.

@michalsn michalsn merged commit 1589d81 into codeigniter4:develop Jul 22, 2020
@michalsn michalsn deleted the fix/entity_fill branch August 8, 2020 13:33
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.

4 participants