Skip to content

Add a filter for serialization attributes#1123

Merged
dunglas merged 2 commits into
api-platform:masterfrom
meyerbaptiste:add_field_filter
May 23, 2017
Merged

Add a filter for serialization attributes#1123
dunglas merged 2 commits into
api-platform:masterfrom
meyerbaptiste:add_field_filter

Conversation

@meyerbaptiste

@meyerbaptiste meyerbaptiste commented May 17, 2017

Copy link
Copy Markdown
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
Comment thread src/Bridge/Doctrine/Orm/Filter/ExistsFilter.php Outdated
{
const QUERY_PARAMETER_KEY = 'exists';

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

@soyuka soyuka May 17, 2017

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've seen this in other filters I think. Do this need to be part of this commit?

@meyerbaptiste meyerbaptiste May 18, 2017

Copy link
Copy Markdown
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.

@soyuka soyuka May 18, 2017

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.

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

@meyerbaptiste meyerbaptiste May 18, 2017

Copy link
Copy Markdown
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

@soyuka soyuka 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.

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

teohhanhui commented May 18, 2017

Copy link
Copy Markdown
Contributor

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

@teohhanhui

teohhanhui commented May 18, 2017

Copy link
Copy Markdown
Contributor

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

@dunglas

dunglas commented May 18, 2017

Copy link
Copy Markdown
Member

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

@meyerbaptiste

meyerbaptiste commented May 18, 2017

Copy link
Copy Markdown
Member Author

Done.

@teohhanhui

teohhanhui commented May 19, 2017

Copy link
Copy Markdown
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

dunglas commented May 22, 2017

Copy link
Copy Markdown
Member

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

dunglas commented May 23, 2017

Copy link
Copy Markdown
Member

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

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@meyerbaptiste @teohhanhui @dunglas @soyuka