-
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.8] Add join
method to collection
#27723
Conversation
join
method to collectionjoin
method to collection
I'll recap from #27657
For reference, times when Oxford commas have been added in Laravel, and Rails |
return ''; | ||
} | ||
|
||
if ($this->count() === 1) { |
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.
Can you cache the result of count
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.
Makes sense, done.
} | ||
|
||
if ($this->count() === 1) { | ||
return $this->last(); |
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.
Can this be cast to string explicitly?
Additionally, as noted in the previous PR for this, can we add support for the Oxford comma? There are two ways I can think to implement this.
Desired result: collect(['a', 'b', 'c']))->join(', ', ', and ')); // returns 'a, b, and c'
collect(['a', 'b']))->join(', ', ', and ', ' and ')); // returns 'a and b'
collect(['a', 'b']))->join(', ', ', and ', true)); // returns 'a and b' Implementation 1: /**
* Join all items from the collection using a string. The final items can use a separate glue string.
*
* @param string $glue
* @param string $finalGlue
* @param string $dyadicGlue
* @return string
*/
public function join($glue, $finalGlue = null, $dyadicGlue = null)
{
if (func_num_args() === 1) {
return $this->implode($glue);
}
$count = $this->count();
if ($count < 2) {
return $this->implode('');
}
if ($count === 2) {
return $this->implode(func_num_args() === 3 ? $dyadicGlue : $finalGlue)
}
$collection = new static($this->items);
$finalItem = $collection->pop();
return $collection->implode($glue).$finalGlue.$finalItem;
} Implementation 2: /**
* Join all items from the collection using a string. The final items can use a separate glue string.
*
* @param string $glue
* @param string $finalGlue
* @param bool $oxfordComma
* @return string
*/
public function join($glue, $finalGlue = null, $oxfordComma = false)
{
if (func_num_args() === 1) {
return $this->implode($glue);
}
$count = $this->count();
if ($count < 2) {
return $this->implode('');
}
if ($count === 2 && $oxfordComma) {
$finalGlue = preg_replace('/^,/', '', $finalGlue);
}
$collection = new static($this->items);
$finalItem = $collection->pop();
return $collection->implode($glue).$finalGlue.$finalItem;
} |
Please use the first option, rather than the second. When dealing with I18N, there are always "awkward" cases, and logic such as "automatically removing one string from another" cannot be guaranteed to work. A simple example might be where the items are a mix of LTR and RTL text, and are being displayed on an LTR page. Your joiners might be |
* @param string $finalGlue | ||
* @return string | ||
*/ | ||
public function join($glue, $finalGlue = '') |
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.
Maybe rename finalGlue
to lastGlue
?
*/ | ||
public function join($glue, $finalGlue = '') | ||
{ | ||
if ($finalGlue === '') { |
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.
Should we change the default value to null
instead of ''
, as this removes the option for having an empty string as your finalGlue
.
I think this really misses the ability to pluck at the same time. Ability to include a readable Don't you think this functionality would be more appropriate as an option for implode? As it stands right now, the both methods would seem half useful. Why not make |
This PR adds documentation to the `join` method: laravel/framework#27723
This PR adds a function to easily use a separate "glue" for the final item.