[12.x] update documentation for custom collections #10948
Conversation
there are 3 ways to define a custom collection, 2 of which are currently documented - `CollectedBy` attribute - override `newCollection()` method - override `$collectionClass` property (undocumented) surprisingly, the undocumented option is arguably the best. it doesn't have the performance hit from Reflection of the attribute, and doesn't force the complete override of a parent method which may miss out on upstream changes. I went aggressive in this commit, and removed the 2 current options in favor of the single best one. However, I could see the argument to keep the "attribute" option, and would add that back in if desired.
|
You've got to be kidding … This is just sabotage at this point … |
|
If you've got a legitimate disagreement, then voice it. I've updated the docs to show the best of the 3 available options. |
|
If you added it, okay, but deleting the rest is very self-righteous. |
|
As @rodrigopedra correctly pointed out in the framework PR, the way the code is currently written it's actually impossible for us to avoid the reflection tax, so my original comment was incorrect. We brainstormed a couple of ways to avoid the reflection while maintaining the Attribute feature, which I may send in. IMO I think it is still best to document this single option so we could possibly phase out the 2 worse options more easily over time. |
|
Reflection could work with caching, but afaik, reflection can't be entirely avoided when using attributes. |
2 participants
there are 3 ways to define a custom collection, 2 of which are currently documented
CollectedByattributenewCollection()method$collectionClassproperty (undocumented)surprisingly, the undocumented option is arguably the best. it doesn't have the performance hit from Reflection of the attribute, and doesn't force the complete override of a parent method which may miss out on upstream changes.
In this PR, I removed the 2 current options in favor of the single best one.
this is a follow up to laravel/framework#58111