-
Notifications
You must be signed in to change notification settings - Fork 11.1k
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
[5.4] Higher-order messages for the collection class #16267
[5.4] Higher-order messages for the collection class #16267
Conversation
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.
It looks interesting :)
public function __get($name) | ||
{ | ||
return $this->collection->{$this->method}(function($value) use ($name) { | ||
return $value->$name; |
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.
Perhaps we shouldn't assume the value will be an object. We also use collections for arrays.
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.
Oh, good point. We could add an offsetGet
method. :)
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.
@franzliedke I fixed this in a separate PR to allow getting by object or array
That's some clever stuff! 😈 |
Woah, this wasn't ready yet. I wanted to add tests, and docs, and an |
'inReverseOrderOf' => 'sortByDesc', | ||
]; | ||
|
||
if (isset($nameToMethod[$name])) { |
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.
am I reading this wrong? shouldn't this be !isset()
?
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.
Damn, that's what I get for refactoring this method at the last moment.
'sum' => 'sum', | ||
'inOrderOf' => 'sortBy', | ||
'inReverseOrderOf' => 'sortByDesc', | ||
]; |
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.
With that array we can't make the $invoices->each->pay();
announced in the LN article . Can we ? We have to do $invoices->do->pay();
instead. Maybe am I missing something somewhere else ? In this case, I think we should use the original methods too.
Thx.
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.
That was just the original implementation with the method names from the article that I linked in the original description. I didn't consider this finished, but Taylor merged it and changed it to what was explained in the Laravel News article.
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.
Yeah I've seen that afterwards. Too bad I liked the aliases you made...
But I've seen too that the merge and the update were fast and uncontrolled ! ^^"
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.
Personally I would prefer $invoices->each()->pay(); format over $invoices->each->pay();
Howdy - long time, no see! :)
I've recently come across an article about a concept called Higher-Order Messages. That particular article was about Ruby, but it intrigued me to implement this for Laravel. And it turns out it's very easy!
This lets you turn some very common collection use-cases into concise one-liners.
filter
exampleIt also works for other collection methods:
This is just a proposal. If you're interested in merging this into Laravel, I'll gladly provide the unit tests and clean up the code. If not, I have saved some time and will probably publish a package.
Naming: I used the conventions from the aforementioned article, but this probably requires some input to make it consistent with the rest of the collection.