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

set _api_write_item_iri only if result object is a subclass of a resource class #2810

Closed
wants to merge 3 commits into from

Conversation

Perf
Copy link

@Perf Perf commented May 20, 2019

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? no
Fixed tickets #2803
License MIT
Doc PR -

Issue happens when output class for a specific operation is not related to a class that represents api resource.
WriteListener tries to set _api_write_item_iri from a result object (defined in output attribute of an operation) but fails as it's not possible.

@maks-rafalko
Copy link
Contributor

Could you please use descriptive commit messages and update the title of this issue so it can be searched/indexed properly in the future? Thanks! :)

@teohhanhui
Copy link
Contributor

The fix is wrong. getIriFromResourceClass gives the collection IRI, not the item IRI.

I think we should just check whether it's a resource item, and skip setting the attribute if it's not. Because we should not make assumptions that won't hold true.

@teohhanhui
Copy link
Contributor

If you don't mind, maybe we could do this after #2797 is merged? I think it'll be easier.

@Perf Perf changed the title fix for #2803 set _api_write_item_iri only if result object is a subclass of a resource class May 20, 2019
@Perf
Copy link
Author

Perf commented May 20, 2019

Could you please use descriptive commit messages and update the title of this issue so it can be searched/indexed properly in the future? Thanks! :)

Hi @borNfreee,
thanks! Title and description updated.

@Perf
Copy link
Author

Perf commented May 20, 2019

The fix is wrong. getIriFromResourceClass gives the collection IRI, not the item IRI.

I think we should just check whether it's a resource item, and skip setting the attribute if it's not. Because we should not make assumptions that won't hold true.

Hi @teohhanhui,
true, I updated PR, no it will set _api_write_item_iri only if result object is a subclass of a resource class.

If you don't mind, maybe we could do this after #2797 is merged? I think it'll be easier.

You know better, I guess ;)

@olivermack
Copy link

Wouldn't it be less restrictive to check if the $controllerResult is just any kind of known ApiResource? With the current state of the PR we'd not be able to link to any other resource type other than the one that has been requested.

E.g. when we dispatch a request for a DTO or any other unidentifiable resource and the DataPersister returns any other applicable ApiResource than given in $attributes['resource_class'] the link may be especially handy - because a client simply could not even assume the result-link.

// ... SNIP
if ($hasOutput) {
    try {
        $request->attributes->set('_api_write_item_iri', $this->iriConverter->getIriFromItem($controllerResult));
    } catch (ResourceClassNotFoundException $e) {
        // swallow
    }
}

Unfortunately the IriConverter bubbles only a simple ApiPlatform\Core\Exception\InvalidArgumentException instead of a (possibly) more suitable ApiPlatform\Core\Exception\ResourceClassNotFoundException.

@karser
Copy link
Contributor

karser commented Jun 14, 2019

Looks like this issue is related #2860

@teohhanhui
Copy link
Contributor

See #2860 (comment)

I think this is not a bug.

@teohhanhui
Copy link
Contributor

Replaced by #2910

@teohhanhui teohhanhui closed this Jul 8, 2019
@teohhanhui teohhanhui self-assigned this Jul 8, 2019
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.

5 participants