[String] Fixed u()->snake(), b()->snake() and s()->snake() methods #57497
Conversation
|
Hey! I see that this is your first PR. That is great! Welcome! Symfony has a contribution guide which I suggest you to read. In short:
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! I am going to sit back now and wait for the reviews. Cheers! Carsonbot |
|
|
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. |
|
Can you amend+push again to trigger tests? Strange they didn't run. |
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"
Done |
|
Thank you @arczinosek. |
|
Hi, I think this introduce a bug/important change in the way snake case is generated. Prior to this PR, Is it wanted @arczinosek @nicolas-grekas ? |
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 | MIT Commits ------- 0e6cd07 normalize underscores in snake()
|
Another regression has been reported: #57612 |
|
see #57615 |
…>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 | MIT This 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"
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 | MIT In 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, #57616 Commits ------- a6d0f3b test: kebab-case to snake_case
[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".