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

Use ExceptionValueMap to store exception values (codes or message) instead of trait #1373

Closed
wants to merge 2 commits into from

Conversation

munkie
Copy link
Contributor

@munkie munkie commented Mar 7, 2016

Use ExceptionValueMap to store map of exception values (codes or messages)
And use it instead of ClassMapHandlerTrait to get code or message associated with exception
In this case resolve logic is incapsulated in ExceptionValueMap and is not exposed into controller or serialize normalizer.

@munkie munkie force-pushed the exception-value-map branch 2 times, most recently from 700b9f5 to b1d505a Compare March 7, 2016 16:32
<argument type="collection"/> <!-- exception codes -->
</service>

<service id="fos_rest.exception.messages_map" class="FOS\RestBundle\Util\ExceptionValueMap">
Copy link
Member

Choose a reason for hiding this comment

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

should be private (public="false")

@GuilhemN
Copy link
Member

GuilhemN commented Mar 7, 2016

Seems really good 👍

@GuilhemN
Copy link
Member

GuilhemN commented Mar 7, 2016

should be rebased on master

@lsmith77
Copy link
Member

lsmith77 commented Mar 7, 2016

do you want to put this into 2.0?

@GuilhemN
Copy link
Member

GuilhemN commented Mar 7, 2016

Yeah it certainly don't affect people and it would be easier to respect bc

@munkie
Copy link
Contributor Author

munkie commented Mar 8, 2016

Rebased.
Made map services private.
Made ExceptionValueMap::resolveClass method private

@GuilhemN
Copy link
Member

GuilhemN commented Mar 8, 2016

Status: Reviewed

@lsmith77 what do you think about removing the required statuses ? It's really annoying to have to rebase a PR each time another one is merged :-/

@lsmith77
Copy link
Member

lsmith77 commented Mar 8, 2016

the conflicts is I think nothing related to the required checks .. this is more that git cannot cleanly merge the source branch into the target branch

@GuilhemN
Copy link
Member

GuilhemN commented Mar 8, 2016

@lsmith77 right here yeah but the other rebase were needed by github, nothing related to git.
Anyway we still can't resolve the conflicts manually as we can't push to the master branch without creating a PR.
(BTW removing the required statuses doesn't mean removing the protection against force pushing)

@GuilhemN GuilhemN mentioned this pull request Mar 9, 2016
GuilhemN added a commit that referenced this pull request Mar 9, 2016
@GuilhemN
Copy link
Member

GuilhemN commented Mar 9, 2016

Merged in #1380 thank you @munkie

@GuilhemN GuilhemN closed this Mar 9, 2016
@lsmith77 lsmith77 removed the in review label Mar 9, 2016
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.

3 participants