Skip to content

Add a filter for serialization attributes #1123

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
May 23, 2017
Merged

Add a filter for serialization attributes #1123

merged 2 commits into from
May 23, 2017

Conversation

meyerbaptiste
Copy link
Member

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #1083
License MIT
Doc PR N/A

soyuka
soyuka reviewed May 17, 2017
View reviewed changes
src/Bridge/Doctrine/Orm/Filter/ExistsFilter.php Outdated
@@ -37,11 +34,6 @@ class ExistsFilter extends AbstractFilter
{
const QUERY_PARAMETER_KEY = 'exists';

public function __construct(ManagerRegistry $managerRegistry, RequestStack $requestStack, LoggerInterface $logger = null, array $properties = null)
Copy link
Member

Choose a reason for hiding this comment

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

I've seen this in other filters I think. Do this need to be part of this commit?

Copy link
Member Author

Choose a reason for hiding this comment

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

No this is the only filter where there is this useless constructor! I will open another PR.

Copy link
Member

Choose a reason for hiding this comment

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

Okay, we have other useless constructors (QueryChecker for example)

Copy link
Member Author

Choose a reason for hiding this comment

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

Not totally useless, the QueryChecker's constructor is private! 😛

soyuka
soyuka approved these changes May 17, 2017
View reviewed changes
Copy link
Member

@soyuka soyuka left a comment

Choose a reason for hiding this comment

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

nice! 👍

@meyerbaptiste meyerbaptiste force-pushed the add_field_filter branch 2 times, most recently from f738976 to d3cd6e5 Compare May 18, 2017 09:21
@teohhanhui
Copy link
Contributor

Perhaps "properties" instead of "fields" to be consistent in our terminology?

@teohhanhui
Copy link
Contributor

But I guess people are more used to "fields"... Maybe we should just keep it this way. 😄

@dunglas
Copy link
Member

dunglas commented May 18, 2017

I'm for property. It's why what we use in API Platform (@ApiProperty) and it's the Hydra terminology.

@meyerbaptiste
Copy link
Member Author

Done.

@teohhanhui
Copy link
Contributor

It's why what we use in API Platform (@ApiProperty) and it's the Hydra terminology.

Yes, but what would people be looking for, when they want that feature? "Fields". It might be worth using the term most people are likely most familiar with.

This was referenced May 20, 2017
Closed
Merged
@meyerbaptiste meyerbaptiste force-pushed the add_field_filter branch 3 times, most recently from ae5a752 to ddb72a3 Compare May 22, 2017 11:43
@dunglas
Copy link
Member

dunglas commented May 22, 2017

Yes, but what would people be looking for, when they want that feature? "Fields". It might be worth using the term most people are likely most familiar with.

It's a doc issue. We need to make it easy to find on the website using both terms, but in the code, consistency is the key. Keep it as is.

@dunglas dunglas merged commit e3c17c8 into api-platform:master May 23, 2017
@dunglas
Copy link
Member

dunglas commented May 23, 2017

Good job!!

@soyuka soyuka mentioned this pull request May 23, 2017
Closed
@meyerbaptiste meyerbaptiste mentioned this pull request May 23, 2017
Closed
@meyerbaptiste meyerbaptiste deleted the add_field_filter branch May 23, 2017 09:06
@teohhanhui teohhanhui mentioned this pull request May 29, 2017
Closed
@meyerbaptiste meyerbaptiste mentioned this pull request Aug 23, 2017
Merged
hoangnd25 pushed a commit to hoangnd25/core that referenced this pull request Feb 23, 2018
@meyerbaptiste @dunglas
Add a filter for serialization attributes (api-platform#1123)
61a9ad9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@soyuka soyuka soyuka approved these changes

@dunglas dunglas dunglas approved these changes

Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@meyerbaptiste @teohhanhui @dunglas @soyuka