Skip to content

Conversation

@timkelty
Copy link
Contributor

@timkelty timkelty commented Feb 20, 2025

Reimplements #54630, now accounting for Eloquent models, which are \Stringable.

Tests now exist for Colletions with \Stringable, \Illuminate\Support\Stringable, and \Illuminate\Database\Eloquent\Model.

@timkelty timkelty changed the title Bugfix/collection implode stringable Fix Collection::implode with \Stringable objects (with exception for Eloquent models) Feb 20, 2025
@taylorotwell
Copy link
Member

This logic is hard to parse.

@taylorotwell taylorotwell marked this pull request as draft February 20, 2025 14:56
@timkelty
Copy link
Contributor Author

Simplified logic to make it easier to parse.

The bigger question for future versions is whether the test for \Stringable or \Illuminate\Support\Stringable should be retained at all.

It seems limiting that Collections of objects that happen to implement \Stringable or \Illuminate\Support\Stringable would have this behavior forced.

@timkelty timkelty marked this pull request as ready for review February 20, 2025 15:42
@timkelty
Copy link
Contributor Author

Closing, as this really would constitute a breaking change.

Any Collections with objects implementing \Stringable, relying on a plucked value, could be inadvertently affected.

@timkelty timkelty closed this Feb 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants