Skip to content

fix property name in ContextAwareFilter to support GraphQL#1743

Closed
mneuhaus wants to merge 2 commits into
api-platform:masterfrom
mneuhaus:fix_graphql_filter
Closed

fix property name in ContextAwareFilter to support GraphQL#1743
mneuhaus wants to merge 2 commits into
api-platform:masterfrom
mneuhaus:fix_graphql_filter

Conversation

@mneuhaus

@mneuhaus mneuhaus commented Mar 2, 2018

Copy link
Copy Markdown
Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #1714
License MIT
Doc PR -

GraphQL filter properties use a underscore instead of dot to separate nested fields, which causes some issues like #1714 where nested filters are not applied. This change replaces the underscore with a dot in the AbstractContextAwareFilter to have one common syntax further down.

GraphQL filter properties use a underscore instead of dot to seperate nested fields, which causes some issues like api-platform#1714 where nested filters are not applied. This change replaces the underscore with a dot in the AbstractContextAwareFilter to have one commen syntax further down.

closes api-platform#1714

@dunglas dunglas left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you add a test to prevent a future regression please?

@dunglas dunglas left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think it's not the good way to fix this.

@mneuhaus

mneuhaus commented Mar 5, 2018

Copy link
Copy Markdown
Author

any suggestion how/were i should fix this? :)

@dunglas

dunglas commented Mar 5, 2018

Copy link
Copy Markdown
Member

Using https://github.com/api-platform/core/blob/2.2/src/Util/RequestParser.php#L39 to retrieve the filters should do the trick.

@Simperfit

Copy link
Copy Markdown
Contributor

@mneuhaus Do you want to try what @dunglas suggested ?

@mneuhaus

mneuhaus commented Apr 6, 2018

Copy link
Copy Markdown
Author

Hey, yes i did, but sadly got stuck in there and did not manage to fix it there :/

@Simperfit

Copy link
Copy Markdown
Contributor

Did you push the WIP ? Ill try to help you ;).

@antograssiot

Copy link
Copy Markdown
Contributor

@Simperfit

Copy link
Copy Markdown
Contributor

This is fixed by @antograssiot. in #1868

Thanks anyway for this PR!

@Simperfit Simperfit closed this Apr 20, 2018
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.

4 participants