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

Feature: resource default config #3135

Closed
wants to merge 2 commits into from
Closed

Feature: resource default config #3135

wants to merge 2 commits into from

Conversation

bpolaszek
Copy link
Contributor

@bpolaszek bpolaszek commented Oct 2, 2019

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #3099
License MIT

This PR introduces a defaults configuration as an implementation of #3099.

Dummy implementation
As discussed offline with @alanpoulain and @dunglas, the implementation has been thought to be as dummy as possible, i.e. any configurable property or attribute in @ApiResource (and even future ones) can be configured on a default basis.

This means this configuration is perfectly correct for all resources:

api_platform:
    defaults:
        attributes:
            mercure: true
            messenger: true

But this one too:

api_platform:
    defaults:
        description: foo

Which means unconfigured @ApiResources will have the same description. That doesn't make sense, but we assume it's up to the developer, not the implementation, to decide what makes sense and what doesn't.

Short-hand operations configuration aren't supported yet
This short-hand configuration:

api_platform:
    defaults:
        itemOperations: ["get"]
        collectionOperations: ["get"]

is currently normalized in a private method from another ResourceMetadataFactory with a higher priority. What are your recommendations to standardize it?

@bpolaszek
Copy link
Contributor Author

As discussed offline with @dunglas, several issues may be raised with this approach:

  • Defaults should be flattened (no more attributes sublevel)
  • snake_case should be the used
  • Using a decorating service with a low priority may prevent it from having the right configuration, as some data is normalized before.
  • Some properties should indeed be prevented from having defaults, e.g. shortName.

I'll open another PR to reflect those changes.

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.

2 participants