Skip to content
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.

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

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Xen3r0
Copy link

@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
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!

src/Context.php Outdated Show resolved Hide resolved
src/Context.php Outdated Show resolved Hide resolved
@Xen3r0
Copy link
Author

Xen3r0 commented Feb 28, 2023

@scyzoryck @jdreesen can you review again please ?

@@ -72,7 +72,13 @@ public function initialize(string $format, VisitorInterface $visitor, GraphNavig
$this->metadataStack = new \SplStack();

if (isset($this->attributes['groups'])) {
$this->addExclusionStrategy(new GroupsExclusionStrategy($this->attributes['groups']));
$strategy = new GroupsExclusionStrategy($this->attributes['groups']);
Copy link
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
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
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.

4 participants