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

Issue #283: Add support for custom serialization groups #287

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kiler129
Copy link
Contributor

@kiler129 kiler129 commented Feb 13, 2019

Q A
Bug fix? no
New feature? yes
BC breaks? no
Related Issue Fix #283
Need Doc update yes

Describe your change

  1. Serialization/normalization context now contains rootEntity containing FQCN of the entity defined for the index
  2. New configuration option serializer_groups allows specifying serialization groups to use on per index basis

What problem is this fixing?

See #283 for detailed description.

Background

In essence this came up with our enterprise integration - I found a fix so I decided to poke around in my free time (so no worry here for copyright, patch is MIT).

@kiler129
Copy link
Contributor Author

@nunomaduro Is there a chance of merging this FR? (trying to decide whatever to wait or maintain a private fork for now ;))

@nunomaduro
Copy link
Contributor

nunomaduro commented Feb 15, 2019 via email

@kiler129
Copy link
Contributor Author

Fridays MUST be read-only :)

Copy link
Contributor

@nunomaduro nunomaduro left a comment

Choose a reason for hiding this comment

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

I need to review this more, were are the comments so far.

tests/TestCase/ConfigurationTest.php Show resolved Hide resolved
(!isset($v['enable_serializer_groups']) || !$v['enable_serializer_groups']);
})
->thenInvalid('In order to specify "serializer_groups" you need to enable "enable_serializer_groups"')
->end()
Copy link
Contributor

Choose a reason for hiding this comment

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

This shouldn't have ->defaultFalse()?

Copy link
Contributor Author

@kiler129 kiler129 Feb 19, 2019

Choose a reason for hiding this comment

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

I don't see a reason why this node should (or can it even?) get a default value of false?
This is just to prevent putting custom groups and wondering why this doesn't work (so really an DX improvement here).

Am I missing something?

src/IndexManager.php Show resolved Hide resolved
src/SearchableEntity.php Show resolved Hide resolved
tests/Normalizer/CommentNormalizer.php Outdated Show resolved Hide resolved
src/SearchableEntity.php Outdated Show resolved Hide resolved
@kiler129
Copy link
Contributor Author

@nunomaduro I addressed the comments - let me know if anything else should be changed.

@kiler129
Copy link
Contributor Author

@nunomaduro Can I do anything to make this issue mergable?

@kiler129
Copy link
Contributor Author

kiler129 commented May 6, 2019

@nunomaduro Any progress on this review?

@nunomaduro
Copy link
Contributor

@kiler129 This feature/pr it's in our todolist. Unfortunately we didn't found time yet for the review. I am sorry.

@chloelbn
Copy link
Contributor

Hey @kiler129 ,

Thanks again for your contribution! Unfortunately we still don't have the time to review it so I'm putting it on hold for the moment.

Thanks for your patience!

@ostrolucky
Copy link
Contributor

PR looks great, unfortunately we use JMS so no use for us. This will definitely need to have conflicts solved before maintainers can have a look here though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Custom Serialization Groups & Access To Root Entity Class On Normalization
4 participants