Skip to content

feat(exclusion): add new option "inheritedGroups" into GroupExclusionStrategy#1474

Open
Xen3r0 wants to merge 2 commits into
schmittjoh:masterfrom
Xen3r0:feat/inherited-groups
Open

feat(exclusion): add new option "inheritedGroups" into GroupExclusionStrategy#1474
Xen3r0 wants to merge 2 commits into
schmittjoh:masterfrom
Xen3r0:feat/inherited-groups

Conversation

@Xen3r0
Copy link
Copy Markdown

@Xen3r0 Xen3r0 commented Feb 14, 2023

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

Questions :

  • How can test attribute inherited_groups in Context.php ?
  • Any help for my first contribution ?

Copy link
Copy Markdown
Collaborator

@scyzoryck scyzoryck left a comment

Choose a reason for hiding this comment

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

Thanks for your fist contribution!
Could you also update documentation about exclusions about those changes, please?
If you need any help, feel free to catch me!

Comment thread src/Context.php Outdated
Comment thread src/Context.php Outdated
@Xen3r0
Copy link
Copy Markdown
Author

Xen3r0 commented Feb 28, 2023

@scyzoryck @jdreesen can you review again please ?

Comment thread src/Context.php

if (isset($this->attributes['groups'])) {
$this->addExclusionStrategy(new GroupsExclusionStrategy($this->attributes['groups']));
$strategy = new GroupsExclusionStrategy($this->attributes['groups']);
Copy link
Copy Markdown
Collaborator

@goetas goetas Feb 28, 2023

Choose a reason for hiding this comment

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

why not

$context = // ... get the context somehow
$g = new GroupsExclusionStrategy(['a', 'b'], true);
$context->addExclusionStrategy($g);

?

In that way we do not need to add the enableInheritGroups method in the context. Ideally the context should know the less possible about the various exclusion strategies.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Ok, i see, but i can't add 2 GroupsExclusionStrategy, one with inheritedGroups = false and an another one with inheritedGroups = true.

How can i configure this option with JMSerializerBundle for example ?

Thanks for you help

@goetas
Copy link
Copy Markdown
Collaborator

goetas commented Feb 28, 2023

I like this because it introduces back #1071 in a backward compat way

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.

5 participants