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

[PHP] Small tweaks for php generator, PHPStan level 1 #7528

Merged
merged 2 commits into from
Oct 7, 2020
Merged

[PHP] Small tweaks for php generator, PHPStan level 1 #7528

merged 2 commits into from
Oct 7, 2020

Conversation

dkarlovi
Copy link
Contributor

Continuation on #7376, fixes two small code issues and a single code style issue.

PR checklist

  • Read the contribution guidelines.
  • Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
  • If contributing template-only or documentation-only changes which will change sample output, build the project beforehand.
  • Run the shell script ./bin/generate-samples.shto update all Petstore samples related to your fix. This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master. These must match the expectations made by your contribution. You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/configs/java*. For Windows users, please run the script in Git BASH.
  • File the PR against the correct branch: master
  • Copy the technical committee to review the pull request if your PR is targeting a particular programming language.

@jebentier (2017/07), @dkarlovi (2017/07), @mandrean (2017/08), @jfastnacht (2017/09), @ackintosh (2017/09) heart, @ybelenko (2018/07), @renepardon (2018/12)

$propertyValue = $data->{$instance::attributeMap()[$property]};
if (isset($propertyValue)) {
if (isset($data->{$instance::attributeMap()[$property]})) {
$propertyValue = $data->{$instance::attributeMap()[$property]};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The previous statement basically made no sense, it's just a null check in disguise.

@@ -63,18 +63,18 @@ use {{invokerPackage}}\ObjectSerializer;
* @param ClientInterface $client
* @param Configuration $config
* @param HeaderSelector $selector
* @param int $host_index (Optional) host index to select the list of hosts if defined in the OpenAPI spec
* @param int $hostIndex (Optional) host index to select the list of hosts if defined in the OpenAPI spec
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Code style fix.

@@ -567,14 +560,16 @@ use {{invokerPackage}}\ObjectSerializer;
}

// for model (json/xml)
if (isset($_tempBody)) {
// $_tempBody is the method argument, if present
{{#bodyParams}}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This one produced nonsensical code unless you have body params:

  1. sets $_tempBody to null
  2. checks if $_tempBody is not null

It can never be not null because the code mutating it is not generated, why are the checks for it generated then?

@dkarlovi
Copy link
Contributor Author

dkarlovi commented Oct 5, 2020

@ybelenko @wing328 WDYT? I have another one in the pipeline once this one is accepted.

@ybelenko
Copy link
Contributor

ybelenko commented Oct 5, 2020

I'm going to check this pr locally today.

}
} elseif (count($formParams) > 0) {
} else
Copy link
Contributor

Choose a reason for hiding this comment

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

This abandoned else really bothers me... Did you forgot to delete it? Or maybe you wanted it as elseif as it was before?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The issue is the if is only needed when you have the body, otherwise I delete it and the elseif becomes if. I don't know how to do it better with Mustache. 🙈

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like I fixed it in my commit.

Copy link
Contributor

@ybelenko ybelenko left a comment

Choose a reason for hiding this comment

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

I don't quite understand the part with the body serialization, looks too complicated to understand it without tests, but I hope you know what you're doing. 👍

@ybelenko ybelenko merged commit b1da096 into OpenAPITools:master Oct 7, 2020
@ybelenko ybelenko added this to the 5.0.0 milestone Oct 7, 2020
@dkarlovi dkarlovi deleted the fix/php-phpstan-level-1 branch October 7, 2020 07:12
@dkarlovi
Copy link
Contributor Author

dkarlovi commented Oct 7, 2020

@ybelenko

Basically, the code does this:

  1. create a variable $_tempBody = null
  2. if some condition in the generator, set the variable to some value
  3. you have a if which checks if ($_tempBody !== null)

But, in case the second condition is not set, you generate code like this:

$_tempBody = null;
// additional code here is not generated
if ($_tempBody !== null) {}

Which is nonsensical, the if will never match since the code mutating $_tempBody is not generated.

What I've done is changed the condition from 2. to generate the if() if the condition is set, but don't generate it if not. That way, the generated code makes sense in both cases and should work exactly as before, but doesn't have "generation sickness" (the code being clunky as a side-effect of it being generated).

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

Successfully merging this pull request may close these issues.

None yet

2 participants