-
Notifications
You must be signed in to change notification settings - Fork 2k
refactor: Rework Entity class
#9878
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
base: 4.7
Are you sure you want to change the base?
Conversation
neznaika0
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An actual note? We can already have the 'attributes' field
michalsn
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some initial comments added.
If you fix a bug, it should be mentioned in the changelog as well.
I've lost the changes. I'll fix it |
6efe618 to
027a0f2
Compare
| */ | ||
| protected function dataCaster(): ?DataCaster | ||
| { | ||
| if (! $this->_cast) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| if (! $this->_cast) { | |
| if ($this->casts === []) { |
IMO, this is a better approach. Since $_cast is true by default, the DataCaster is always instantiated during construction, defeating the optimization purpose.
$this->casts === []: Structural question - "Is there anything defined to cast?"$_cast: Runtime toggle - "Should casting be active right now?"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't find any cases when we need $_cast and an empty array.
I wanted to rely on the array at first, but then I thought you don't often approve of surface changes. So I delved into the code 😄
I'll test the new version.
| ******* | ||
|
|
||
| - **Cookie:** The ``CookieInterface::EXPIRES_FORMAT`` has been changed to ``D, d M Y H:i:s T`` to follow the recommended format in RFC 7231. | ||
| - **Entity:** The protected property ``CodeIgniter\Entity\Entity::$dataCaster`` can be nullable. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| - **Entity:** The protected property ``CodeIgniter\Entity\Entity::$dataCaster`` can be nullable. | |
| - **Entity:** The protected property ``CodeIgniter\Entity\Entity::$dataCaster`` type has been changed from ``DataCaster`` to ``?DataCaster`` (nullable). |
This should go under "Method Signature Changes" section, or even better, we can create "Property Signature Changes" subsection within that category.
Description
I keep track of entity changes. At the moment, I don't see any conflicts when using 'attributes' as the column name.
There was also a bug with the restore of
$_cast.Question: Do I need to get rid of the DataCaster object? For example, I completely disabled casting in the Entity and transform it into a Model. But the object is always being created.
I could, under the condition
$_cast = false, not create it or destroy it when calling$this->cast(false). We can simplify by callinggetDataCaster()"on the fly", although it's no better to create an object every time. Suggestions?It is also planned to update the DateCast to use the interface, since it does not always set DateTime, but also DateTimeImmutable.
PR should be taken after #9877 But we can discuss it now.
Checklist: