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

[Serializer] CamelCaseToSnakeCaseNameConverter upper camel case not working #21399

Closed
markusu49 opened this issue Jan 25, 2017 · 10 comments
Closed

Comments

@markusu49
Copy link
Contributor

Q A
Bug report? yes
Feature request? no
BC Break report? no
RFC? no
Symfony version all supported

The CamelCaseToSnakeCaseNameConverter provides a constructor argument to specify whether to use lower camel case or not. Denormalizing upper camel case works fine, but normalizing ThisIsATest returns _this_is_a_test (should be this_is_a_test).

@theofidry
Copy link
Contributor

theofidry commented Jan 25, 2017

@markusu49 this is happening because ThisIsATest is not camelCase but PascalCase, see https://3v4l.org/qTioM.

@markusu49
Copy link
Contributor Author

Isn't this naming Microsoft-specific (see Wikipedia)? What is the lowerCamelCase attribute for then?

@theofidry
Copy link
Contributor

Some people and organizations, notably Microsoft,[4] use the term camel case only for lower camel case.

It's not limited to Microsoft but more generally in programming languages.

@markusu49
Copy link
Contributor Author

markusu49 commented Jan 25, 2017

PascalCase is just another name for upper camel case - people often call things differently. However, the denormalize method supports it:

return $this->lowerCamelCase ? lcfirst($camelCasedName) : $camelCasedName;

Why shouldn't the normalize method, too?

@chalasr
Copy link
Member

chalasr commented Jan 25, 2017

@theofidry as your last quote points it out, there is indeed two kind of camel case: lower and upper, PascalCase being a synonym of upper camelCase (also known as StudlyCaps). I was not aware of that but according to https://fr.wikipedia.org/wiki/CamelCase, it seems true.

The $lowerCamelCase constructor argument let me understand the same as @markusu49 i.e. you can chose between both syntaxes using this normalizer so the result it expects looks legit to me.

@theofidry
Copy link
Contributor

@chalasr IMO it's up to discussion, and tbh I don't feel strongly about it, but if the denormalize method supports it, the normalize one should as well indeed.

@chalasr
Copy link
Member

chalasr commented Jan 25, 2017

Also I would intuitively prefer a CamelCaseToSnakeCaseNameConverter and a PascalCaseToSnakeCaseNameConverter than only the former handling both

@robfrawley
Copy link
Contributor

robfrawley commented Jan 25, 2017

Isn't "lower camel case" simply "camel case", whereas "upper camel case" is "pascal case"? The class/arguments are unclear in their current form. Two separate converter classes would be more appropriate.

Edit: We posted at the same time, but indeed, @chalasr, that would be much more intuitive.

@markusu49
Copy link
Contributor Author

How should we handle edge cases then, like passing ThisIsATest to the (lower) camel case converter? It currently converts to _this_is_a_test, but it isn't reversible, as the unit tests make sure that _kevin_dunglas converts to _kevinDunglas.

@chalasr
Copy link
Member

chalasr commented Jan 26, 2017

@markusu49 Your fix is legit on 2.7. If we want to separate, we have to deprecate this argument and introduce the new converter, that must be done on master, in another PR.

fabpot added a commit that referenced this issue Feb 16, 2017
…(markusu49)

This PR was merged into the 2.7 branch.

Discussion
----------

[Serializer] fix upper camel case conversion (see #21399)

| Q             | A
| ------------- | ---
| Branch?       | 2.7
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | ?
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #21399
| License       | MIT

Improve upper camel case support of CamelCaseToSnakeCaseConverter (`ThisIsATest` now converts to `this_is_a_test` instead of `_this_is_a_test`).

Commits
-------

81e771c [Serializer] fix upper camel case conversion (see #21399)
@fabpot fabpot closed this as completed Feb 16, 2017
fabpot added a commit that referenced this issue Feb 16, 2017
* 2.7:
  Permit empty suffix on Windows
  [Console][Table] fixed render when using multiple rowspans.
  add docblocks for Twig url and path function to improve ide completion
  check for circular refs caused by method calls
  [Serializer] fix upper camel case conversion (see #21399)
  [DI] Auto register extension configuration classes as a resource
  [Console] Updated phpdoc on return types
fabpot added a commit that referenced this issue Feb 16, 2017
* 2.8:
  Permit empty suffix on Windows
  [Console][Table] fixed render when using multiple rowspans.
  add docblocks for Twig url and path function to improve ide completion
  check for circular refs caused by method calls
  [Serializer] fix upper camel case conversion (see #21399)
  [DI] Auto register extension configuration classes as a resource
  [Console] Updated phpdoc on return types
fabpot added a commit that referenced this issue Feb 16, 2017
* 3.2:
  Permit empty suffix on Windows
  fixed CS
  [FrameworkBundle] Remove unused import
  [Console][Table] fixed render when using multiple rowspans.
  add docblocks for Twig url and path function to improve ide completion
  check for circular refs caused by method calls
  [Serializer] fix upper camel case conversion (see #21399)
  [DI] Auto register extension configuration classes as a resource
  [Console] Updated phpdoc on return types
This was referenced Mar 6, 2017
sandergo90 pushed a commit to sandergo90/symfony that referenced this issue Jul 4, 2019
sandergo90 pushed a commit to sandergo90/symfony that referenced this issue Jul 4, 2019
* 2.7:
  Permit empty suffix on Windows
  [Console][Table] fixed render when using multiple rowspans.
  add docblocks for Twig url and path function to improve ide completion
  check for circular refs caused by method calls
  [Serializer] fix upper camel case conversion (see symfony#21399)
  [DI] Auto register extension configuration classes as a resource
  [Console] Updated phpdoc on return types
sandergo90 pushed a commit to sandergo90/symfony that referenced this issue Jul 4, 2019
* 2.8:
  Permit empty suffix on Windows
  [Console][Table] fixed render when using multiple rowspans.
  add docblocks for Twig url and path function to improve ide completion
  check for circular refs caused by method calls
  [Serializer] fix upper camel case conversion (see symfony#21399)
  [DI] Auto register extension configuration classes as a resource
  [Console] Updated phpdoc on return types
sandergo90 pushed a commit to sandergo90/symfony that referenced this issue Jul 4, 2019
* 3.2:
  Permit empty suffix on Windows
  fixed CS
  [FrameworkBundle] Remove unused import
  [Console][Table] fixed render when using multiple rowspans.
  add docblocks for Twig url and path function to improve ide completion
  check for circular refs caused by method calls
  [Serializer] fix upper camel case conversion (see symfony#21399)
  [DI] Auto register extension configuration classes as a resource
  [Console] Updated phpdoc on return types
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants