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

Don't typehint the SchemaFactory decorator by its implementation, but by its interface #3417

Merged

Conversation

arnedesmedt
Copy link
Contributor

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

Enable decorating schema factory by it's interface.

The Hydra Json Schema Factory could not be decorated with a custom SchemaFactoryInterface.
The JsonSchemaFactory was used as a default. I've changed the type hint to the interface so everybody can now generate their own SchemaFactory implementations.

@teohhanhui
Copy link
Contributor

Uhh... Thanks for catching this! I believe it's an unintended mistake.

@teohhanhui
Copy link
Contributor

Rebase needed because I've just merged another PR. Could you take care of it, or would you like me to do it?

@teohhanhui teohhanhui added the bug label Feb 27, 2020
@teohhanhui teohhanhui changed the title Don't typehint the Schemafactory decorator by it's implementation, but Don't typehint the SchemaFactory decorator by its implementation, but its interface Feb 27, 2020
@arnedesmedt arnedesmedt force-pushed the enableDecoratingSchemaFactory branch from c1d9e0f to b4aea13 Compare February 27, 2020 16:25
@arnedesmedt
Copy link
Contributor Author

I've rebased it and wrote an if round some JsonSchemaFactory specific code.

$schemaFactory->addDistinctFormat('jsonld');

if ($schemaFactory instanceof BaseSchemaFactory) {
$schemaFactory->addDistinctFormat('jsonld');
Copy link
Contributor

@teohhanhui teohhanhui Feb 27, 2020

Choose a reason for hiding this comment

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

This is very weird indeed...

Looks like a design problem to me @dunglas

Copy link
Contributor Author

@arnedesmedt arnedesmedt Mar 2, 2020

Choose a reason for hiding this comment

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

@teohhanhui Is it possible to add this and create a new issue to solve the design issue?

@teohhanhui teohhanhui force-pushed the enableDecoratingSchemaFactory branch from b4aea13 to a23a67c Compare March 2, 2020 16:10
@teohhanhui teohhanhui changed the title Don't typehint the SchemaFactory decorator by its implementation, but its interface Don't typehint the SchemaFactory decorator by its implementation, but by its interface Mar 2, 2020
@teohhanhui teohhanhui merged commit 6578c8b into api-platform:2.5 Mar 2, 2020
@teohhanhui
Copy link
Contributor

Thanks @arnedesmedt! 🎉

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

Successfully merging this pull request may close these issues.

2 participants