[PHP] Replace File.seperator with slashes in PHP projects #2004

[jfastnacht]

PR checklist

• Read the contribution guidelines.
• Ran the shell script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh and ./bin/security/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in .\bin\windows\.
• Filed the PR against the correct branch: master, 3.4.x, 4.0.x. Default: master.
• Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

#2004

@jebentier @dkarlovi @mandrean @jfastnacht @ackintosh @ybelenko @renepardon

[ybelenko]

Great!

You fixed annoying bug, when output on Windows always differ with double forward slashes.
Now I can work on Windows and my generated output will always match CI results.
Checked locally with PHP Slim Server generator.

Thanks a lot!

[ybelenko]

@jfastnacht
Maybe we should also swap File.separator with / in AbstractPhpCodegen?

openapi-generator/modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/AbstractPhpCodegen.java

Line 57 in 9fe3c76

protected String apiDocPath = docsBasePath + File.separator + apiDirName;

Should I change all File.separarator in PhpSlimServerCodegen too?

[jfastnacht]

@jfastnacht
Maybe we should also swap File.separator with / in AbstractPhpCodegen?
openapi-generator/modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/AbstractPhpCodegen.java

Line 57 in 9fe3c76

protected String apiDocPath = docsBasePath + File.separator + apiDirName;
Should I change all File.separarator in PhpSlimServerCodegen too?

Probably, but we should be careful due to possible side effects. I can check on Sunday or Monday, unless you want to change it yourself.

[jfastnacht]

@ybelenko I couldn't find a problematic part in PhpSlimServerCodegen. If you don't see any other problem it's ready to merge from my perspective.

[ybelenko]

@jfastnacht I've checked your PR again, PhpSlimServerCodegen works correct.

Now I do understand why we can't change File.separator in apiFileFolder() and modelFileFolder() methods. These values are used by codegen itself and they needs to be responsive to user OS.
We can change only codegen properties within mustaches, php scripts and docs, where forward slash is completely safe.

But... why DefaultCodegen contains forward slash?

openapi-generator/modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultCodegen.java

Lines 632 to 646 in 8a8a184

	public String apiFileFolder() {
	return outputFolder + "/" + apiPackage().replace('.', '/');
	}
	public String modelFileFolder() {
	return outputFolder + "/" + modelPackage().replace('.', '/');
	}
	public String apiTestFileFolder() {
	return outputFolder + "/" + testPackage().replace('.', '/');
	}
	public String modelTestFileFolder() {
	return outputFolder + "/" + testPackage().replace('.', '/');
	}

[jfastnacht]

Now I do understand why we can't change File.separator in apiFileFolder() and modelFileFolder() methods. These values are used by codegen itself and they needs to be responsive to user OS.
We can change only codegen properties within mustaches, php scripts and docs, where forward slash is completely safe.

Yeah, that's what I'm trying to seperate.

But... why DefaultCodegen contains forward slash?

Other languages might be making it differently, so I can only talk for PHP right now.

[ybelenko]

openapi-generator/modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultCodegen.java

Lines 632 to 646 in 8a8a184

	public String apiFileFolder() {
	return outputFolder + "/" + apiPackage().replace('.', '/');
	}
	public String modelFileFolder() {
	return outputFolder + "/" + modelPackage().replace('.', '/');
	}
	public String apiTestFileFolder() {
	return outputFolder + "/" + testPackage().replace('.', '/');
	}
	public String modelTestFileFolder() {
	return outputFolder + "/" + testPackage().replace('.', '/');
	}

@wing328 Is this correct implemetation? Seems to me that File.separator is more safe for DefaultCodegen.

[jfastnacht]

@ybelenko Can you solve this in a seperate issue? I know that this is important, but it's leaving the scope of the issue #2004 this PR was originally opened for.

[ybelenko]

@jfastnacht Totally agree with you.

I won't merge this PR until @ackintosh approved it anyway. His opinion is more relevant because PHP Client codegen is used by larger community.

[wing328]

@jfastnacht thanks for the PR.

@ybelenko thanks for reviewing the change.

If no further feedback on this PR, I'll merge it tomorrow (Tue)

cc @ackintosh

[ackintosh]

Thanks for the great job! ✨