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

[11.x] Allow Str::limit to include ellipsis in length calculation #48772

Closed
wants to merge 3 commits into from

Conversation

inxilpro
Copy link
Contributor

@inxilpro inxilpro commented Oct 18, 2023

Right now Str::limit() checks the limit before appending the $end (three dots, or a faux-ellipsis, by default) to the string. This can result in strings that are truncated and over the limit…

Before:

Str::limit('Hello world', 10);

// Results in "Hello worl..." which is 13 characters in total

This PR includes the length of the $end argument in the calculation:

After:

Str::limit('Hello world', 10);

// Results in "Hello w..." which is 10 characters in total, including the three dots

@inxilpro
Copy link
Contributor Author

Also, I'm happy to change this to 10.x since it's barely a breaking change, but I PR'd against 11.x just to be safe.

@deleugpn
Copy link
Contributor

Seems like the additional parameter is overly protective. Targeting 11.x, I would argue that adding an upgrade note that explains the change and suggests users that if they want the previous behavior they should increment the length by 3 characters or by the size of the $end parameter that they are passing.

Point being, this change as it currently stands is an opt-in change on a future major release. Given how small of a BC this is we could simplify by still targeting 11.x but making it opt-out instead by intructing users to change the limit size.

@inxilpro
Copy link
Contributor Author

Point being, this change as it currently stands is an opt-in change on a future major release. Given how small of a BC this is we could simplify by still targeting 11.x but making it opt-out instead by intructing users to change the limit size.

Yeah, I was being very careful. But I tend to agree. I'll change it to the new default and see what happens :)

@taylorotwell
Copy link
Member

Some of this behavior seems kinda weird? I think I will just stick with how it is.

Your PR:

image

Current behavior:

image

@inxilpro
Copy link
Contributor Author

Haha, I did not account for negative overflow. I may re-open with that fixed…

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