Skip to content

Conversation

@jderusse
Copy link
Member

@jderusse jderusse commented Dec 1, 2025

From AWS spec's properties we tried to normalize these name to camelCase.

For some cases, (when the name contains multiple consecutive UpperCases) where the generator is not able to generate the CamelCase value by itself, we maintain a list of (opinionated) exceptions telling that we shoiuld use the form "first char upper, rest lower". ie ETag become Etag.

We also maintain a list of "ignored" cases where multiple consecutive Upper cases are fine.
ie XOffset stays XOffset.

The issue, is: By mistake, we also convert the second list to the form "first char upper, rest lower".

This PR fix that.

This might not be a BC break, because PHP is not strict about case on methods.. But I wonder if it worth it...

@jderusse jderusse force-pushed the fix-camel branch 4 times, most recently from 4f94d18 to 6e5255b Compare December 3, 2025 08:36
if (preg_match('/[A-Z]{2,}/', $propertyName)) {
$propertyName = strtr($propertyName, $ignored);
if (preg_match('/[A-Z]{2,}/', $propertyName)) {
$propertyNameWithoutIgnored = strtr($propertyName, array_fill_keys($ignored, ''));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This replacement might miss other cases where we would have another capital letter just before or after an ignored group.
The cause of the bug is that this validation overrides $propertyName instead of using a different variable name (as done here with $propertyNameWithoutIgnored). The fix is to rename the variable without changing the way its content is generated (and keeping the existing structure in $ignored)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactored:
I kept the $ignored as a list to avoid typo and ease maintenance.
But I used array_fill_keys($ignored, 'Xx') to avoid collision with neighbors.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants