Tried to fix https://github.com/api-platform/core/issues/1025 #1046
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I don't think that these are all occurrences where the wrong resourceClass is passed via the context. There might be more, especially regarding setting values. I haven't tried that |
soyuka
reviewed
Apr 12, 2017
| @@ -60,7 +60,7 @@ public function supportsNormalization($data, $format = null) | |||
| */ | |||
| public function normalize($object, $format = null, array $context = []) | |||
| { | |||
| $resourceClass = $this->resourceClassResolver->getResourceClass($object, $context['resource_class'] ?? null, true); | |||
| $resourceClass = $this->resourceClassResolver->getResourceClass($object, get_class($object), true); | |||
There was a problem hiding this comment.
This is wrong, the resourceClassResolver already does this. If you want to fix this, you should try to fix it in the ResourceClassResolver class directly.
There was a problem hiding this comment.
Oh, yes I see.
Then I guess it is sufficient to pass null as resourceClass parameter
soyuka
reviewed
Apr 12, 2017
| @@ -79,7 +79,7 @@ public function supportsNormalization($data, $format = null) | |||
| */ | |||
| public function normalize($object, $format = null, array $context = []) | |||
| { | |||
| $resourceClass = $this->resourceClassResolver->getResourceClass($object, $context['resource_class'] ?? null, true); | |||
| $resourceClass = $this->resourceClassResolver->getResourceClass($object, get_class($object), true); | |||
|
This will still break things, I don't think it's the correct way of fixing the inheritance issue. Quoting my comment on api-platform/api-platform#277 : Then there is #1025. This is indeed a bug where the |
|
I am myself not sure if it's the correct way to fix this either. You guys wanted me to create the WIP pull request, so it might be easier to track down the problem. The thing is, that these changes fixed the issue for me in my project. For #1025 as well as for #1043. Though these two issues seem not to be related. I never wanted to say that this is the right fix for it... Yes it might break things. I am waiting for it in my project. |
Yes indeed, that's good to help us find the best solution together :D.
By breaking, I mean that our test suite won't pass with your changes, it's therefore creating bugs. Instead of trying to fix this directly, I would first add a failing behat test. This will then be the starting point to fix the issue. Though, if there are two different issues we need to do this one issue at a time! |
|
The tests look almost green but the 500 error thrown in the Behat test is weird. Do you have an idea of the problem @nik-lampe? You can get a better stack trace by running |
|
@dunglas It appears that this test case fails because of these changes. While these changes fix my failing test case in #1063, it looks like it breaks something else. Click to expand Scenario: Filter with a raw URL # features/hydra/collection.feature:283 |
|
Fixed through #1063 thanks @nik-lampe for investigating/testing! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is my WIP Pull Request adressing issue #1025
I am not sure, if this does not break something else...