[PHP] Generates invalid code due to property renaming

[philip]

Description

Generated PHP code can create invalid PHP code when converting property names to methods, if the property names are similar except one also contains one or more underscore characters. For example, helloworld and hello_world.

After generating both petstore and testpetstore (see below), here's part of the diff:

SwaggerClient-php/lib/Model/Pet.php

@@ -98,6 +102,8 @@
     protected static $setters = array(
         'id' => 'setId',
         'name' => 'setName',
+        'hello_world' => 'setHelloWorld',
+        'helloworld' => 'setHelloworld',
         'tag' => 'setTag'
     );

@@ -113,6 +119,8 @@
     protected static $getters = array(
         'id' => 'getId',
         'name' => 'getName',
+        'hello_world' => 'getHelloWorld',
+        'helloworld' => 'getHelloworld',
         'tag' => 'getTag'
     );

In other words, Pet.php redeclares getHelloWorld() and setHelloWorld() twice in Pet.php, which is a fatal error:

     /**
+     * Gets hello_world
+     * @return string
+     */
+    public function getHelloWorld()
+    {
+        return $this->container['hello_world'];
+    }
+
+    /**
+     * Sets hello_world
+     * @param string $hello_world
+     * @return $this
+     */
+    public function setHelloWorld($hello_world)
+    {
+        $this->container['hello_world'] = $hello_world;
+
+        return $this;
+    }
+
+    /**
+     * Gets helloworld
+     * @return string
+     */
+    public function getHelloworld()
+    {
+        return $this->container['helloworld'];
+    }
+
+    /**
+     * Sets helloworld
+     * @param string $helloworld
+     * @return $this
+     */
+    public function setHelloworld($helloworld)
+    {
+        $this->container['helloworld'] = $helloworld;
+
+        return $this;
+    }
+
+    /**
      * Gets tag
      * @return string
      */

It's not important that the method's case is slightly different. This also means tests break due to redeclared methods:

SwaggerClient-php/test/Model/PetTest.php

+     * Test attribute "hello_world"
+     */
+    public function testPropertyHelloWorld()
+    {
+
+    }
+
+    /**
+     * Test attribute "helloworld"
+     */
+    public function testPropertyHelloworld()
+    {
+
+    }
+
+    /**

I do see that a ensureUniqueParams configuration option exists but it's different. Issue #2766 refers to modelPropertyNaming but PHP does not have this, nor do I think using such an option should be required to fix this. PHP does have variableNamingConvention but that's for variables.

Swagger-codegen version

2.2.1

Swagger declaration file content or url

This can be reproduced using a slightly modified petstore.json with the following change:

--- petstore.json   2017-01-12 13:10:54.000000000 -0800
+++ testpetstore.json   2017-01-12 13:19:08.000000000 -0800
@@ -123,6 +123,12 @@
         "name": {
           "type": "string"
         },
+        "hello_world": {
+          "type": "string"
+        },
+        "helloworld": {
+          "type": "string"
+        },
         "tag": {
           "type": "string"
         }

Command line used for generation

swagger-codegen generate \ --input-spec ./testpetstore.json \ --lang php \ --output /tmp/testpetstore

Steps to reproduce

1. Add two property names where the only difference is one contains an underscore, such as helloworld and hello_world.
2. Generate PHP code
3. Execute code or tests
4. See PHP Fatal error: Cannot redeclare ...

Suggest a Fix

Check for conflicts before generating the code. If two generated method names will be the same, then make one different somehow. I do not know the renaming logic behind ensureUniqueParams but suspect the same or similar logic could be used for methods.

It might be worth having generate echo a warning/notice when methods end up being renamed.

Related

Might be related to Issue #2766

[wing328]

@philip thanks for reporting the issue. I agree with you that ideally codegen should show better warning message if two parameter names happen to be the same after processing.

One solution is to use the --reserved-words-mappings introduced in https://github.com/swagger-api/swagger-codegen/pull/4480, e.g.

java -jar swagger-codegen-cli.jar generate -i pet.yaml -l php --reserved-words-mappings id=identifier -o output

which will map id to identifier.

Please pull the latest master to give it a try.

[philip]

@wing328

Unfortunately that is not an available workaround as it appears specific to the reserved words assigned to each language, and it doesn't seem to affect method names. Please let me know if I misunderstand, though, as I'm a swagger_codegen newbie.

As a side note, the --reserved-words-mappings option doesn't seem to work with -l php, or at least I can't get it to do anything for reserved words like array. That is, assuming these are PHP's reserved words. I can get it to work fine for -l objc using these keywords, such as id and readwrite.

So while --reserved-words-mappings solves a similar problem, it seems different. But, if something like --custom-mappings did exist then it'd have to change both the name and associated method name as the latter is what breaks the code. In this sense, defining both hello_world and helloworld parameters should be possible without both generating the likes of gethelloworld(). I don't think an option like --custom-mappings should be required for this though, as it'd mean user's having to manually maintain a list of potential conflicts. But, the flexibility could help allow related workarounds in the future.

Here's copy-n-paste friendly code to reproduce the original problem that uses a modified pet store gist for simpler testing. As shown earlier, the only change is the addition of the helloworld and hello_world parameters to petstore.json:

java -jar modules/swagger-codegen-cli/target/swagger-codegen-cli.jar generate \
 --input-spec https://gist.githubusercontent.com/philip/115e4aa0303e492413b4c0eff004d4b0/raw/1f8cb1d6b7ae870e1e9b3994b2d64dbd05e2904e/petstore-helloworld.json \
 --lang php \
 --output /tmp/php-client

Then, the simplest way to see the problem:

$ php -l /tmp/php-client/SwaggerClient-php/lib/Model/Pet.php

PHP Fatal error:  Cannot redeclare Swagger\Client\Model\Pet::getHelloworld() in /tmp/php-client/SwaggerClient-php/lib/Model/Pet.php on line 248
...

[wing328]

@philip you're right. Is it correct to say that renmaing the parameter names with conflicts is not an option in your case?

[philip]

@wing328 Thanks to this bug, problems were discovered in our source swagger files where model values were mistakingly merged. So although both foo_bar and foobar exist, they do not belong in the same group. Fixing this at the source means we no longer experience this bug. So thanks to swagger_codegen output, we've made this and several other improvements to the sources.

Fixing this bug is now a low priority for us. I'm not sure what could be added to verbose logs to help similar situations , whether it means showing all conversions, and/or show when '_' characters are in the names (which eventually are removed in associated method names as fooBar, which can be confusing), or what part of all this is language specific. Maybe much of this is simply a matter of writing additional language-specific swagger_codegen documentation, or encouraging folks like me to find and read it :)

[wing328]

@philip thanks for sharing more. We'll treat this with low priority as well since this kinda issue seems very unlikely to occur (assuming the spec is correct).

[peter-fan-cn]

I also have a conflict issue as there is an attribute named modelName in my model properties. but the getModelName method has been defined in the ModelInterface

[hakimdjabri]

many methods are redeclared in each lib classes, this error exist for each class PHP Fatal error: Cannot redeclare Swagger\Client\Model\xxxxx.php

[mschop]

@wing328 Any Updates on this? All *Api-Classes have the same problem. The create{EntityName} method is redeclared in line 381/101.