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.

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

Revert "[11.x] Use Str::wrap() instead of nesting Str::start() inside Str::finish()" #54528

Merged
merged 3 commits into from
Feb 10, 2025

Conversation

shaedrich
Copy link
Contributor

#53987 potentially changes the output of Illuminate\Mail\Mailables\Headers::referencesString()

As @tontonsb explained in #53987 (comment), previously, already wrapped strings would not be wrapped again, but now, they would, resulting in two leading and two trailing angle brackets.

This deviation wasn't discovered by a test, so I added one.

@shaedrich shaedrich changed the title Revert #53987 Revert "[11.x] Use Str::wrap() instead of nesting Str::start() inside Str::finish()" Feb 8, 2025
@@ -95,7 +95,7 @@ public function text(array $text)
public function referencesString(): string
{
return (new Collection($this->references))
->map(fn ($messageId) => Str::wrap($messageId, '<', '>'))
->map(fn ($messageId) => Str::of($messageId)->start('<')->finish('>'))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not reversing, this is adding a Stringable instance overhead on each $references element.

Previous code used nested calls to Str::start() and Str::finish()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, it reverses the functionality which is now asserted by the added test, not necessarily the syntax, which is more readable imo.

@taylorotwell taylorotwell merged commit 814054e into laravel:11.x Feb 10, 2025
44 checks passed
@shaedrich shaedrich deleted the revert-pr-53987 branch February 10, 2025 11:39
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.

3 participants