Added Model extender, integration tests #2100
Conversation
… relation to other models
…, deprecated related events
|
For the Default Attributes one, do we want to allow extension developers to use stuff like settings there? We could pass a predetermined number of things (translator, settings, URL Generator), and have them use those for the default attributes. |
|
Not for now, I'd say. We can consider that when the use-case pops up. Until then it feels just like wild guessing. 😉 |
…, and belongsToMany, as well as corresponding integration tests.
|
|
||
| return $dates[$class]; | ||
| return array_merge($this->dates, Arr::get(static::$dateAttributes, static::class, [])); |
There was a problem hiding this comment.
Should we keep the cache semantics of the previous implementation - so that the event dispatch and the array merge only happens once per class per request?
(Not sure how often this is called internally... it might not be relevant any longer when the event dispatching is gone.)
There was a problem hiding this comment.
I'll take care of this!
There was a problem hiding this comment.
Played around with it. Static caching would cause problems in tests (uh oh), and I'm not too worried about it once the event dispatching is gone, so I'm leaving this as is.
- Remove unused private attributes - Complete docblocks - Add scalar type hints - Format code - Reorder methods Refs #2100.
- Format code - Reorder methods - Test a different scenario to avoid the use of sleep() Refs #2100.
We determined that child classes are not properly affected when extending the parent classes. Refs #2100.
|
I'm only catching up now with those features. I understand you added a way to specify new date attributes on the model, but not casts ? I'm pretty sure casts would be useful. Should we make an issue for that ? |
|
The goal here is to move to the extenders as public API (no functionality was changed), but I agree that would be useful. |
Fixes part of #1891
Changes proposed in this pull request:
Reviewers should focus on:
Anything I'm missing?
Confirmed
composer test).