Skip to content

[11.x] slug() with alphanumeric separator #54446

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

Closed
wants to merge 3 commits into from

Conversation

faissaloux
Copy link
Contributor

@faissaloux faissaloux commented Feb 3, 2025

Fixes #54444

This PR made slug() fallback to default separator (-) when an alphanumeric separator been passed.

Examples

Before

str('nad')->slug('az'); // nazd

After

str('nad')->slug('az'); // nad

str('hello world')->slug('e'); // hello-word
str('hello world')->slug('2'); // hello-word

@shaedrich
Copy link
Contributor

I don't like these silent fallbacks that might not be intuitive and therefore easily overlooked when something doesn't work as expected. Maybe, throwing an exception is more intuitive.

@rodrigopedra
Copy link
Contributor

I agree with @shaedrich, silent fallbacks are not intuitive at all.

And this approach also does not solve the issue described, as the issue reporter expects to be using a localized slug in a different language, and the proposed solution would mask the issue further as they would expect language-specific characters to be treated in that language.

For example, the word "soccer" in Russian is written as "футбольный" (according to Google Translate, I don't speak Russian). It would get different slugs depending on the language parameter:

$ php artisan tinker
Psy Shell v0.12.7 (PHP 8.2.27 — cli) by Justin Hileman
> str('футбольный')->slug()
= Illuminate\Support\Stringable {#6608
    value: "futbolnyi",
  }

> str('футбольный')->slug('ru')
= Illuminate\Support\Stringable {#6632
    value: "frutbolnyi",
  }

> str('футбольный')->slug('-')
= Illuminate\Support\Stringable {#6626
    value: "futbolnyi",
  }

> str('футбольный')->slug(language: 'ru')
= Illuminate\Support\Stringable {#6612
    value: "futbolnyy",
  }

Blindly replacing ru by - when the language parameter is not specified correctly would result in an unexpected slug, as outlined in the examples above.

I commented in the issue, recommending the user to use a named parameter to properly specify the language parameter:

#54444 (comment)

@vlakoff
Copy link
Contributor

vlakoff commented Feb 4, 2025

The use case is incorrect due to an erroneous parameter, and the proposed change would not solve the issue, but rather create additional problems. This should be closed as invalid.

@faissaloux
Copy link
Contributor Author

I made it now throws an exception when separator is alphanumeric.

Thanks all for your feedback!

@vlakoff
Copy link
Contributor

vlakoff commented Feb 5, 2025

I'm not certain if this check is adequate…

A more adequate check could be:

if (preg_match('![\pL\pN\s]!u', $separator)) { /* throw exception */ }

Using [\pL\pN\s] or [\pL\pN], as I'm unsure about spaces.

But again, I'm not convinced it would be helpful.

In practice, a check enforcing that mb_strlen($separator) === 1 would be more helpful (because multi-character separators are not supported and, arguably, it might be unexpected in plausible situations).

@vlakoff
Copy link
Contributor

vlakoff commented Feb 5, 2025

Though, if you look at this comment in the previous ticket:

One could propose a PR to assert a separator only has a single character.

On the other hand, people could be relying on the current behavior to generate strings like this:

$ php artisan tinker
Psy Shell v0.12.7 (PHP 8.2.27 — cli) by Justin Hileman
> str('PHP is great!')->slug('-=-')
= Illuminate\Support\Stringable {#6609
    value: "php-=-is-=-great",
  }

Multi-character separators "just work" by luck (at least in some cases), and some people might be using it.

Therefore, I would suggest changing nothing, i.e., don't add any check. Neither for alphanumeric separators nor for multi-character separators.

As a reminder, the issue was initially due to a completely erroneous parameter… we don't have to handle that.

@shaedrich
Copy link
Contributor

it might be unexpected in plausible situations

We could always have some kind of setting to enable/disable this

@faissaloux faissaloux deleted the slug-alphanumeric branch February 5, 2025 21:01
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.

Weird behavior of Str::slug in particular language
5 participants