NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[String] Fixed u()->snake(), b()->snake() and s()->snake() methods#57497

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

nicolas-grekas merged 1 commit intosymfony:5.4fromarczinosek:fix_57464

Jun 28, 2024

Merged

[String] Fixed u()->snake(), b()->snake() and s()->snake() methods#57497

nicolas-grekas merged 1 commit intosymfony:5.4fromarczinosek:fix_57464

Jun 28, 2024

Conversation

Copy link

Contributor

arczinosek commentedJun 22, 2024

Q	A
Branch?	5.4
Bug fix?	yes
New feature?	no
Deprecations?	no
Issues	Fix#57464
License	MIT

[String] ByteString::snake and AbstractUnitcodeString::snake methods doesn't convert all uppercase words in the right way.

For example, string "GREAT SYMFONY" is converted to "greatsymfony" instead of "great_symfony".

arczinosek requested review fromOskarStark,chalasr,dunglas andyceruto ascode owners

June 22, 2024 20:19

carsonbot added Status: Needs Review Bug String labels

Jun 22, 2024

carsonbot added this to the7.2 milestone

Jun 22, 2024

Copy link

carsonbot commentedJun 22, 2024

Hey!

I see that this is your first PR. That is great! Welcome!

Symfony has acontribution guide which I suggest you to read.

In short:

Always add tests
Keep backward compatibility (seehttps://symfony.com/bc).
Bug fixes must be submitted against the lowest maintained branch where they apply (seehttps://symfony.com/releases)
Features and deprecations must be submitted against the 7.2 branch.

Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change.

When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

arczinosek changed the base branch from7.2 to5.4

June 22, 2024 20:20

Copy link

ContributorAuthor

arczinosek commentedJun 22, 2024•
edited
Loading

fabbot suggests me a fix with coding standards, but it's for a different part of the code. Should I apply?
Because of my mistake milestone was probably incorrectly set to 7.2 instead of 5.4, sorry

xabbuh modified the milestones:7.2,5.4

Jun 22, 2024

Copy link

Member

alexandre-daubois commentedJun 23, 2024

You can ignore fabbot suggestion that are unrelated to your code.

I've got a remark about this change. This is a behavior update, which seems indeed like a bug fix. However, many people could rely on the current behavior and this change could definitely bring a lot of unexpected breakages wherever this is used. This would mean a lot of BC breaks are to be expected which is not permitted by patch and minor versions bugfixes are targeting. 🤔 Because of this, I'm not sure this can be accepted as-is. Curious to have the opinion of someone else.

symfony deleted a comment fromcarsonbot

Jun 23, 2024

Copy link

Member

nicolas-grekas commentedJun 24, 2024

Can you amend+push again to trigger tests? Strange they didn't run.
On my side this is a legit bugfix.

[String] Fix *String::snake methods

f93113e

The ByteString::snake and AbstractUnitcodeString::snake methods had a bug that caused incorrect string conversion results for all uppercase words separated by space or "_" character.Ex. "GREAT SYMFONY" was converted to "greatsymfony" instead of "great_symfony"

arczinosek force-pushed thefix_57464 branch fromd64a190 tof93113eCompare

June 26, 2024 19:54

Copy link

ContributorAuthor

arczinosek commentedJun 26, 2024

Can you amend+push again to trigger tests? Strange they didn't run. On my side this is a legit bugfix.

Done

nicolas-grekas approved these changes

Jun 28, 2024

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels

Jun 28, 2024

xabbuh approved these changes

Jun 28, 2024

View reviewed changes

Copy link

Member

nicolas-grekas commentedJun 28, 2024

Thank you@arczinosek.

nicolas-grekas merged commit6517957 intosymfony:5.4

Jun 28, 2024

This was referencedJun 28, 2024

Release v5.4.41#57579

Merged

Release v6.4.9#57580

Merged

Copy link

Contributor

VincentLanglet commentedJun 28, 2024•
edited
Loading

Hi, I think this introduce a bug/important change in the way snake case is generated.

Prior to this PR,symfony is great was snaked tosymfony_is_great.
Now it'ssymfony_____is_____great.

Is it wanted@arczinosek @nicolas-grekas ?

This was referencedJun 28, 2024

Release v7.0.9#57581

Merged

Release v7.1.2#57582

Merged

xabbuh mentioned this pull request

Jun 29, 2024

[String] Normalize underscores in snake()#57594

Merged

nicolas-grekas added a commit that referenced this pull request

Jun 29, 2024

bug#57594[String] Normalize underscores in snake() (xabbuh)

4e57b84

This PR was merged into the 5.4 branch.Discussion----------[String] Normalize underscores in snake()| Q             | A| ------------- | ---| Branch?       | 5.4| Bug fix?      | yes| New feature?  | no| Deprecations? | no| Issues        |Fix#57497 (comment)| License       | MITCommits-------0e6cd07 normalize underscores in snake()

Copy link

Member

nicolas-grekas commentedJun 29, 2024

@VincentLanglet see#57594

jmsche mentioned this pull request

Jul 1, 2024

[String] snake() method changes in 7.1.2 breaks symfony/ux-translator#57612

Closed

Copy link

Member

derrabus commentedJul 1, 2024

Another regression has been reported:#57612

xabbuh mentioned this pull request

Jul 1, 2024

[String] fix handling of punctuation characters in snake()#57615

Closed

Copy link

Member

xabbuh commentedJul 1, 2024

see#57615

nicolas-grekas added a commit to nicolas-grekas/symfony that referenced this pull request

Jul 1, 2024

Revert "bugsymfony#57497[String] Fixed u()->snake(), b()->snake() a…

3b4cbfe

…nd s()->snake() methods (arczinosek)"This reverts commit6517957, reversingchanges made to572ce41.

nicolas-grekas mentioned this pull request

Jul 1, 2024

[String] Revert "Fixed u()->snake(), b()->snake() and s()->snake() methods"#57616

Merged

fabpot added a commit that referenced this pull request

Jul 1, 2024

bug#57616[String] Revert "Fixed u()->snake(), b()->snake() and s()-…

e3625f2

…>snake() methods" (nicolas-grekas)This PR was squashed before being merged into the 5.4 branch.Discussion----------[String] Revert "Fixed u()->snake(), b()->snake() and s()->snake() methods"| Q             | A| ------------- | ---| Branch?       | 5.4| Bug fix?      | yes| New feature?  | no| Deprecations? | no| Issues        |Fix#57612| License       | MITThis PR reverts#57497 for BC reasons, but keeps the test cases we added in the process. Those test cases allowed to spot a real issue where the ascii and unicode implementations didn't agree on the resulting camel case for `SYMFONY IS GREAT`. Both implementations now result in `SYMFONYISGREAT` (likely introduced in#47423).Commits-------6397c38 [String] Revert "Fixed u()->snake(), b()->snake() and s()->snake() methods"

wmouwen mentioned this pull request

Jul 2, 2024

[String] test: kebab-case to snake_case#57628

Merged

fabpot added a commit that referenced this pull request

Jul 6, 2024

minor#57628[String] test: kebab-case to snake_case (wmouwen)

737d519

This PR was merged into the 5.4 branch.Discussion----------[String] test: kebab-case to snake_case| Q             | A| ------------- | ---| Branch?       | 5.4| Bug fix?      | yes?| New feature?  | no| Deprecations? | no| Issues        | -| License       | MITIn Symfony 7.1.1, `kebab-case` strings casted to `snake_case` properly. The changes made in 7.1.2 broke this functionality and kept it `kebab-case`.It would be nice to have a `kebab-case` test added to the list to clearly define the expected behavior.Relates to:#57497,#57612,#57616Commits-------a6d0f3b test: kebab-case to snake_case