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

Ensure that api:openapi:export produces JSON with unescaped forward slashes #3368

Conversation

Ocramius
Copy link
Contributor

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tickets
License MIT
Doc PR

This ensures that the generated OpenAPI schema JSON dump does not contain forward-slashes, such as "#\/foo\/bar\/MyModel", which confuses tooling relying on direct string match in referenced models (most notably, the IntelliJ OpenAPI/Swagger tooling)

use Symfony\Component\Console\Output\BufferedOutput;
use Symfony\Component\Serializer\Normalizer\NormalizerInterface;

class SwaggerCommandUnitTest extends KernelTestCase
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: a unit test was much simpler and efficient to write - the current integration test is not something I could work with anyway, since I don't run MongoDB on any system I maintain (including my dev environment) by choice

Copy link
Member

Choose a reason for hiding this comment

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

Actually there was a an attempt to remove the hard dev dependency to MongoDB (#2703), but it has been added again by #2843 (an issue with PHPStan it seems).

By default though, the Behat tests are run without MongoDB.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I hope that can be achieved again at some point: for now, I'm happy with adding a unit test, and providing more unit tests in future, shall I encounter isolated issues, like this one.

Copy link
Contributor

@teohhanhui teohhanhui Feb 17, 2020

Choose a reason for hiding this comment

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

PHP extensions are no longer required to be loaded - thanks to Roave/BetterReflection and PhpStorm stubs!

https://github.com/phpstan/phpstan/releases/tag/0.12.4

We should be able to do this once we upgrade to phpstan ^0.12.4! 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately, we still can't remove the MongoDB packages, because we'd get errors such as:

 ------ -------------------------------------------------------------------------------------------- 
  Line   src/Bridge/Doctrine/Common/Util/IdentifierManagerTrait.php (in context of anonymous class)  
 ------ -------------------------------------------------------------------------------------------- 
  47     Class Doctrine\ODM\MongoDB\DocumentManager not found.                                       
  86     Call to static method hasType() on an unknown class Doctrine\ODM\MongoDB\Types\Type.        
  87     Call to static method getType() on an unknown class Doctrine\ODM\MongoDB\Types\Type.        
 ------ --------------------------------------------------------------------------------------------

😞

Copy link
Contributor

Choose a reason for hiding this comment

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

@ondrejmirtes Do you perhaps have any suggestions on how this could be resolved? I see there are MongoDB ODM stubs in https://github.com/phpstan/phpstan-doctrine/tree/master/stubs

Is there a way to tell phpstan that these types "exist" purely based on the stubs?

Copy link
Contributor

Choose a reason for hiding this comment

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

See #3431 for an attempt.

Choose a reason for hiding this comment

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

Is there a way to tell phpstan that these types "exist" purely based on the stubs?

You'd have to make the class available for the autoloader. See: https://phpstan.org/user-guide/autoloading

@Ocramius Ocramius force-pushed the fix/dump-openapi-schema-without-escaping-slashes branch from 0dd8feb to 0d31896 Compare January 29, 2020 10:45
@alanpoulain
Copy link
Member

Couldn't this be a BC if the client has been relying on the escaped slashes?

@Ocramius
Copy link
Contributor Author

Doubt that, but there is indeed a change in dumped definitions. Forward slashes do not impose an escaping sequence risk.

@dunglas dunglas merged commit 66cc242 into api-platform:2.5 Jan 31, 2020
@dunglas
Copy link
Member

dunglas commented Jan 31, 2020

Thanks @Ocramius!

@Ocramius Ocramius deleted the fix/dump-openapi-schema-without-escaping-slashes branch February 12, 2020 14:01
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.

None yet

5 participants