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.

Sign up for GitHub

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

Jump to bottom

[5.4] Higher-order messages for the collection class #16267

Merged
merged 1 commit into from
Nov 4, 2016
Merged

[5.4] Higher-order messages for the collection class #16267

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 52 additions & 0 deletions src/Illuminate/Support/Collection.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1266,6 +1266,25 @@ public function toBase()
return new self($this);
}

public function __get($name)
{
$nameToMethod = [
'where' => 'filter',
'unless' => 'reject',
'do' => 'each',
'extract' => 'map',
'sum' => 'sum',
'inOrderOf' => 'sortBy',
'inReverseOrderOf' => 'sortByDesc',
];
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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 ! ^^"

Copy link

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();


if (isset($nameToMethod[$name])) {
Copy link
Contributor

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()?

Copy link
Contributor Author

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.

throw new \Exception('Not accessible.');
}

return new HigherOrderProxy($this, $nameToMethod[$name]);
}

/**
* Determine if an item exists at an offset.
*
Expand Down Expand Up @@ -1350,3 +1369,36 @@ protected function getArrayableItems($items)
return (array) $items;
}
}

class HigherOrderProxy
{
/**
* @var Collection
*/
protected $collection;

/**
* @var string
*/
protected $method;

public function __construct(Collection $collection, $method)
{
$this->collection = $collection;
$this->method = $method;
}

public function __call($name, $arguments)
{
return $this->collection->{$this->method}(function($value) use ($name, $arguments) {
return call_user_func_array([$value, $name], $arguments);
});
}

public function __get($name)
{
return $this->collection->{$this->method}(function($value) use ($name) {
return $value->$name;
Copy link

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.

Copy link
Contributor Author

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. :)

Copy link
Contributor

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

});
}
}