[Serializer] Ignore first uppercase character

[timhovius]

Q	A
Branch?	2.7, 2.8, 3.0 or 3.1
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	-
License	MIT
Doc PR	-

The string CamelCaseString was converted to _camel_case_string. The first character must be ignored.

[sstok]

Please use the strict comparison 👍

[timhovius]

I will do 👍

[sstok]

Please add a test to proof that's its fixed.

Status: needs work

[timhovius]

Added a unit test dataProvider value 👍

[fabpot]

should be merged in 2.7.

[timhovius]

Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'firstCharacterNoUnderscore'
+'FirstCharacterNoUnderscore'

Should I write an another dataProvider, or lowercase the string first?

[timhovius]

It's fixed now 😃

[xabbuh]

we use Yoda conditions in the Symfony code base: 0 !== $i

[phansys]

This could be simplified with something like this, saving one call to ctype_upper():

if (0 !== $i && ($lcPropertyName = strtolower($propertyName[$i])) !== $propertyName[$i]) {
    $snakeCasedName .= '_'.$lcPropertyName;
}

BTW, if you don't agree with this approach, please move the cheaper check left.

[dunglas]

What's the benefit of replacing a call to ctype_upper by a call to strtolower?

[phansys]

Now, each iteration calls ctype_upper() and strtolower(); with my proposal we can omit the call to ctype_upper().

[phansys]

The complete if/else structure I'm proposing:

if (($lcPropertyName = strtolower($propertyName[$i])) !== $propertyName[$i] && 0 !== $i) {
    $snakeCasedName .= '_'.$lcPropertyName;
} else {
    $snakeCasedName .= $lcPropertyName;
}

[dunglas]

Ok, got it. Your improvement looks good!

The code can be more readable:

$lcPropertyName = strtolower($propertyName[$i]);
if (0 !== $i && $lcPropertyName !== $propertyName[$i]) {
    $snakeCasedName .= '_'.$lcPropertyName;
} else {
    $snakeCasedName .= $lcPropertyName;
}

[GuilhemN]

@phansys @dunglas something even more efficient and clearer imo could be:

$len = strlen($propertyName) - 1;
$lowerCasedName = strtolower($propertyName);

// First character taken as is
$snakeCasedName = isset($lowerCasedName[0]) ? $lowerCasedName[0] : '';

for ($i = 1; $i < $len; ++$i) {
    // If uppercase
    if ($lowerCasedName[0] !== $propertyName[$i]) {
       $snakeCasedName .= '_';
    }

    $snakeCasedName .= $lowerCasedName[$i];
}

Of course the easiest way to decide which structure to use would be to run a benchmark against each implementation.

Edit: missed your comment @dunglas

[GuilhemN]

I ran a small php benchmarks with 100 000 occurrences and different names:
with Foo:

test_original             : 0.099 sec.
test_energetick           : 0.047 sec.
test_dunglas              : 0.077 sec.

with aLongNameIsVeryLong:

test_original             : 0.558 sec.
test_energetick           : 0.364 sec.
test_dunglas              : 0.480 sec.

The results aren't that significant so choose what you think is the more readable :-)

Edit: I used this

[dunglas]

@timhovius Do you have some time to finish this PR?

[timhovius]

@dunglas Yes of course. I will choose one of the solutions above.

[fabpot]

@timhovius What's the status of this PR?

[timhovius]

@fabpot I'm working on it

[timhovius]

It's fixed now, I implemented the solution of @dunglas. The solution of @Ener-Getick was failing.

[dunglas]

The fix looks good to me. But can you take into account @Ener-Getick's comment for the test? Thanks.

[timhovius]

Wat do you Mean @dunglas?

[DavidBadura]

@timhovius this: #19112 (comment) ;)

[dunglas]

@timhovius any update? Do you want me to finish this PR?

[timhovius]

I fixed the unit tests

[stof]

your new tests don't seem to be covering this issue. They look like they cover exactly the same cases than existing cases

[timhovius]

Yes but the new tests are failing. Try it.

[fabpot]

I've just tested and the two new tests pass without any changes, which makes sense to me as there are very similar to the existing ones.

[DavidBadura]

it should be:

array('first_character_no_underscore', 'FirstCharacterNoUnderscore'),
array('tim_hovius', 'TimHovius'),

[fabpot]

Closing as there is no feedback and the added tests do not reveal anything to be fixed. Feel free to reopen with unit tests that demonstrate the issue. Thanks.