Add a filter for serialization groups by meyerbaptiste · Pull Request #1111 · api-platform/core

meyerbaptiste · 2017-05-10T15:02:37Z

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

dunglas · 2017-05-11T08:13:08Z

+ *
+ * @author Baptiste Meyer <baptiste@les-tilleuls.coop>
+ */
+class GroupFilter implements FilterInterface


I followed the same standards as ORM filters: not final + protected methods/attributes.

So @dunglas, should we keep consistency with ORM filters or should I follow your requested changes?

ORM filters support only the array syntax. So, same for this new one.

No, I talk about final + protected @dunglas 😋

dunglas · 2017-05-11T08:13:15Z

+ */
+class GroupFilter implements FilterInterface
+{
+    protected $overrideDefaultGroups;


dunglas · 2017-05-11T08:13:32Z

+     */
+    public function apply(Request $request, bool $normalization, array $attributes, array &$context)
+    {
+        if (empty($groups = $this->extractGroups($request))) {


empty is useless.

dunglas · 2017-05-11T08:13:46Z

+            return;
+        }
+
+        if (!$this->overrideDefaultGroups && !empty($context['groups'])) {


dunglas · 2017-05-11T08:14:23Z

+     *
+     * @return array
+     */
+    protected function extractGroups(Request $request): array


dunglas · 2017-05-11T08:14:57Z

+        $groups = [];
+
+        if (null !== $groupsParameter = $request->query->get($this->parameterName)) {
+            if (is_string($groupsParameter)) {


Why supporting both syntax? We only support the second one in other filters.

I don't think we should explode or do any transformation on query parameters. Just let symfony do the work here.

I'd propose:

$groups = $request->query->get($this->parameterName);

This method can then be removed as this one line should be enough.

I followed what you described in your issue: ?groups=foo,bar,baz and I added the array syntax. So... should I implement the issue syntax or array syntax or both?

After a second though, I was wrong :) We should just support the array syntax, just like for other fields (consistency...).

dunglas · 2017-05-11T08:15:53Z

+            $resourceFilters = $resourceMetadata->getItemOperationAttribute($attributes['item_operation_name'], 'filters', [], true);
+        }
+
+        if (empty($resourceFilters)) {


empty is useless

meyerbaptiste · 2017-05-11T12:50:30Z

+            return;
+        }
+
+        if (!$this->overrideDefaultGroups && isset($context['groups']) && is_array($context['groups'])) {


About is_array($context['groups']), I'm not really sure how we should handle the case where $context['groups'] exists and is not an array:

cast $context['groups'] to array and apply the filter

do not apply the filter when $overrideDefaultGroups is true or false (so in all cases)

apply the filter when $overrideDefaultGroups is true

apply the filter and overwrite the value of $context['groups'] in all cases (current behavior)

WDYT?

What is $context['groups'] if not an array? I think we should cast is as one.

What is $context['groups'] if not an array?

I don't know, it's just an assumption!

Should be a string, therefore we could cast it as array.

dunglas · 2017-05-15T15:47:29Z

+ *
+ * @author Baptiste Meyer <baptiste.meyer@gmail.com>
+ */
+final class SerializerContextBuilderFilter implements SerializerContextBuilderInterface


SerializerFilterContextBuilder?

teohhanhui · 2017-05-15T16:51:52Z

To restate what I've raised in #1082 (comment), I think we should not introduce misfeatures like this...

teohhanhui · 2017-05-15T16:54:57Z

If we are to have a "property group" filter, then the "property groups" MUST be exposed as resources. That's the way to keep things as hypermedia.

We need to always keep these principles in mind, as we claim that API Platform is a hypermedia framework / library. 😄

teohhanhui

Please take #1111 (comment) into account. Which also means this needs to be generalized into a "property groups" filter, not a "serializer groups" filter (implementation detail).

dunglas · 2017-05-17T09:00:44Z

@teohhanhui I'm not sure to get exactly what you have in mind? Can you give us some details?
For me, this filter is a purely technical feature to save bandwidth. It's one of the most wanted feature by our customers and I often see projects having this kind of filters (as custom filters).

It's also a 100% optin filter.

dunglas · 2017-05-17T09:02:28Z

Regarding the implementation details leak, this is not true. As stated just before, filter is 100% optional and must be enabled manually by the developper. When doing this, you know that the groups will be exposed publicly, and you need to take care of this. It's the developper responsibility.

teohhanhui · 2017-05-18T09:50:14Z

Yeah, I guess it must be very hard to expose the property groups as resources.

But I wish we could change "groups" to "property_groups"?

teohhanhui · 2017-05-18T09:51:49Z

When I said it's a leak of implementation detail, I meant that these groups are not defined anywhere. They are not exposed as resources, they are not in the Swagger doc. So they're magical as far as the client is concerned... Out-of-band information which is a big nono.

dunglas · 2017-05-18T11:28:23Z

I agree about the out of band information. However this is not the case the fields filter, so it should be privileged when developing hypermedia API.

About the name, I prefer groups (shorter is better), but it should be configurable, and it is :)

meyerbaptiste · 2017-05-23T09:03:35Z

Merged with #1123, see e3c17c8.

meyerbaptiste force-pushed the add_group_filter branch 3 times, most recently from 1af71a4 to 9694e58 Compare May 10, 2017 15:56

dunglas requested changes May 11, 2017

View reviewed changes

meyerbaptiste force-pushed the add_group_filter branch 3 times, most recently from 9caf397 to abe708b Compare May 11, 2017 12:41

meyerbaptiste commented May 11, 2017

View reviewed changes

soyuka mentioned this pull request May 13, 2017

have handle on GET/POST/DELETE/PUT method api-platform/api-platform#296

Closed

soyuka added the enhancement label May 15, 2017

meyerbaptiste force-pushed the add_group_filter branch from abe708b to 44cdc17 Compare May 15, 2017 14:14

dunglas reviewed May 15, 2017

View reviewed changes

teohhanhui suggested changes May 15, 2017

View reviewed changes

meyerbaptiste force-pushed the add_group_filter branch 3 times, most recently from 61b97bb to cb4d458 Compare May 18, 2017 09:19

meyerbaptiste force-pushed the add_group_filter branch 4 times, most recently from b2df66a to 3d71742 Compare May 22, 2017 13:26

meyerbaptiste closed this May 23, 2017

meyerbaptiste force-pushed the add_group_filter branch from 3d71742 to e3c17c8 Compare May 23, 2017 08:41

meyerbaptiste deleted the add_group_filter branch May 23, 2017 08:42

meyerbaptiste mentioned this pull request May 23, 2017

Add docs for serialization filters api-platform/docs#211

Closed

Uh oh!

Uh oh!

Conversation

meyerbaptiste commented May 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

meyerbaptiste May 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

meyerbaptiste May 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

meyerbaptiste May 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

meyerbaptiste May 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

teohhanhui commented May 15, 2017

Uh oh!

teohhanhui commented May 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

teohhanhui left a comment

Choose a reason for hiding this comment

Uh oh!

dunglas commented May 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dunglas commented May 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

teohhanhui commented May 18, 2017

Uh oh!

teohhanhui commented May 18, 2017

Uh oh!

dunglas commented May 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

meyerbaptiste commented May 23, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

meyerbaptiste commented May 10, 2017 •

edited

Loading

meyerbaptiste May 11, 2017 •

edited

Loading

meyerbaptiste May 11, 2017 •

edited

Loading

meyerbaptiste May 11, 2017 •

edited

Loading

meyerbaptiste May 12, 2017 •

edited

Loading

teohhanhui commented May 15, 2017 •

edited

Loading

dunglas commented May 17, 2017 •

edited

Loading

dunglas commented May 17, 2017 •

edited

Loading

dunglas commented May 18, 2017 •

edited

Loading