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

[BUG]: Camelize helper does not camelCase, and also fails with space delimiters #15850

Closed
lcdennison opened this issue Jan 2, 2022 · 6 comments · Fixed by #15851 or #15875
Closed

[BUG]: Camelize helper does not camelCase, and also fails with space delimiters #15850

lcdennison opened this issue Jan 2, 2022 · 6 comments · Fixed by #15851 or #15875
Assignees
Labels
5.0 The issues we want to solve in the 5.0 release bug A bug report enhancement Enhancement to the framework status: medium Medium

Comments

@lcdennison
Copy link

Describe the bug
The Camelize string helper fails on a few fronts. See obvious issues below.

To Reproduce
Steps to reproduce the behavior:

## Issue #1: Outputting pascal case, not camel case
echo (new \Phalcon\Support\Helper\Str\Camelize())('customer-session'); // default delimiters are "-_"
// outputs "CustomerSession"

// This is PascalCase, not camelCase. The correct result should be "customerSession" if you're calling it a "camelize" function.

## Issue #2: Using space delimiter consumes capital characters
echo (new \Phalcon\Support\Helper\Str\Camelize())('customer Session', ' -_'); // Note the additional space in the delimiters
echo (new \Phalcon\Support\Helper\Str\Camelize())('customer-Session', ' -_'); // Note the additional space in the delimiters
// both statements output "CustomerEssion"

// The correct result is "customerSession" for both. Note this does NOT happen if the string
//   is "customer-session" or "customer session" (i.e. if the "S" is not capital, it does not get removed from the string)

Expected behavior
I use the following function, which converts the above test strings to customerSession as expected in all cases.

// camelCase:
lcfirst(str_replace(' ', '', ucwords(preg_replace('/[^a-zA-Z0-9\x7f-\xff]++/', ' ', $input))));

If you want PascalCase and snake_case as well:

// PascalCase
str_replace(' ', '', ucwords(preg_replace('/[^a-zA-Z0-9\x7f-\xff]++/', ' ', $input))); // same as above, just without the lcfirst()
// snake_case
strtolower(preg_replace('/(?<!^)[A-Z]/', '_$0', $input));

Details

  • Phalcon version: 5.0.0-beta1
  • PHP Version: 8.0.14
@lcdennison lcdennison added bug A bug report status: unverified Unverified labels Jan 2, 2022
@niden
Copy link
Sponsor Member

niden commented Jan 2, 2022

@lcdennison We have a few things going on here.

For v4 and before the camelize method was using a built in method (written in C) in Zephir. For v5 I rewrote the method to a non pure C implementation, with the later goal to translate that to PHP.

Running your examples with the v3/v4 version of camelize produces:

-'customer-session' : +'CustomerSession'
-'customer-Session' : +'CustomerSession'
-'customer Session' : +'CustomerSession'

So for the v4 version of this method and before you get CustomerSession for all examples. The particular method has not changed for prior versions so it was always returning the first character capitalized.

Running this with the current code, gives the results you gave and for sure the last case (" -_") produces the wrong result, removing the S from Session.

I can correct at least the last example but I am afraid if I change this method, a lot of current applications will break because for better or worse, this method was always returning the first character capitalized.

Thoughts?

@niden
Copy link
Sponsor Member

niden commented Jan 2, 2022

We could also introduce one more parameter in the __invoke to do the lower case for the first letter. This way we can have the current functionality but also allow for new one. Additional helpers can be created for pure pascal or snake case

@niden niden self-assigned this Jan 2, 2022
@niden niden added 5.0 The issues we want to solve in the 5.0 release enhancement Enhancement to the framework status: medium Medium and removed status: unverified Unverified labels Jan 2, 2022
@niden niden added this to Working on it in Phalcon Roadmap Jan 2, 2022
niden added a commit to niden/phalcon that referenced this issue Jan 2, 2022
…seFirst if need be; Added PascalCase, SnakeCase and KebabCase helpers

Related phalcon/cphalcon#15850

Refs phalcon#6.0.x
niden added a commit to niden/phalcon that referenced this issue Jan 2, 2022
…seFirst if need be; Added PascalCase, SnakeCase and KebabCase helpers

Related phalcon/cphalcon#15850

Refs phalcon#6.0.x
niden added a commit to phalcon/phalcon that referenced this issue Jan 3, 2022
…seFirst if need be; Added PascalCase, SnakeCase and KebabCase helpers

Related phalcon/cphalcon#15850

Refs #6.0.x
@niden niden mentioned this issue Jan 3, 2022
5 tasks
@niden niden linked a pull request Jan 3, 2022 that will close this issue
5 tasks
@niden
Copy link
Sponsor Member

niden commented Jan 3, 2022

Resolved in #15851

@niden niden closed this as completed Jan 3, 2022
Phalcon Roadmap automation moved this from Working on it to Implemented Jan 3, 2022
@lcdennison
Copy link
Author

lcdennison commented Jan 5, 2022

@niden Sorry for the delayed reply. I certainly understand about not wanting to break existing usages that rely on the capitalized first letter. The changes you committed seem to solve everything nicely. For anyone that wants to make Camelize truly camelCase, they can pass the $lowerFirst = true flag on that invocation. Or additionally, for new codebases, just extend the Camelize class to set $lowerFirst = true by default and use that extended class instead of the base Phalcon one.

I also see a few extra helper classes for pascal, snake, and kebab. Thanks for that!

@lcdennison
Copy link
Author

@niden One minor bug/inconsistency I think with your fix, didn't notice before. The "Kebab" and "Snake" case delimiters are swapped. The comment for kebab says "kebab-case" but uses the "_" delimiter:

* Converts strings to kebab-case style

return implode("_", output);

Similarly, the comment for snake says "snake_case" but uses the "-" delimiter:

* Converts strings to snake_case style

return implode("-", output);

I believe those delimiters just need to be swapped (and the corresponding tests updated).

@niden niden reopened this Jan 25, 2022
@niden niden mentioned this issue Jan 26, 2022
5 tasks
@niden niden linked a pull request Jan 26, 2022 that will close this issue
5 tasks
@niden
Copy link
Sponsor Member

niden commented Jan 26, 2022

@niden One minor bug/inconsistency I think with your fix, didn't notice before. The "Kebab" and "Snake" case delimiters are swapped. The comment for kebab says "kebab-case" but uses the "_" delimiter:

* Converts strings to kebab-case style

return implode("_", output);

Similarly, the comment for snake says "snake_case" but uses the "-" delimiter:

* Converts strings to snake_case style

return implode("-", output);

I believe those delimiters just need to be swapped (and the corresponding tests updated).

@lcdennison

Thank you for the heads up. Resolved in #15875

@niden niden closed this as completed Jan 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5.0 The issues we want to solve in the 5.0 release bug A bug report enhancement Enhancement to the framework status: medium Medium
Projects
Archived in project
Phalcon Roadmap
  
Implemented
Development

Successfully merging a pull request may close this issue.

2 participants