Skip to content

Ignore property that does not exist for interfaces (identifiers for interfaces) #2086

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

Conversation

Nek-
Copy link
Contributor

@Nek- Nek- commented Jul 10, 2018

Q A
Bug fix? yes/no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets Related to #1574
License MIT
Doc PR N/A

Motivations

TL;DR: Before: no "item" route working because we can't define an identifier. After: we can define a property for an interface and it will not be considered as "not found property".

  1. It makes no sense to check if an interface exists in case it's an interface ;
  2. If the property is not recognized as one and there is no identifier, then API Platform will not be able to call the DataProvider for a given item and interface as a resource is an incomplete feature.

Usage

Me\InterfaceAsAResource:
    properties:
         # Will work even if foo is not defined in the interface!
         foo:
             identifier: true

What's next?

If you like this kind of features, I will probably work on "virtual properties" next. I see something like

Me\InterfaceAsAResource:
    properties:
         foo:
             identifier: true
             proxy_method: 'customMethodToGetFoo'

What do you think? Maybe you want a separate RFC issue to discuss it?

@Nek- Nek- force-pushed the feature/ignore-property-exist-test-for-interfaces branch from 11851ca to 5f88305 Compare July 10, 2018 08:24
@Nek- Nek- force-pushed the feature/ignore-property-exist-test-for-interfaces branch from 5f88305 to 6332272 Compare July 16, 2018 09:03
@teohhanhui
Copy link
Contributor

I thought we already support virtual properties?

@Nek-
Copy link
Contributor Author

Nek- commented Jul 16, 2018

@teohhanhui interesting. I was surprised to not find the feature. I'm curious to know where it is documented.

@teohhanhui
Copy link
Contributor

@Nek- I think there's no documentation, not even tests for it. 😆 🙈

But looking at the code (and as I've heard from other's experience), it should work. (Not when using the Extractor factory, obviously, as you've discovered.)

@Nek-
Copy link
Contributor Author

Nek- commented Jul 16, 2018

In any case, it doesn't invalidate this feature as I understand. Are failures on CI related? (due to inversion of condition?)

@teohhanhui
Copy link
Contributor

CI failures are unrelated and should be fixed after #2106 is merged.

@Nek- Nek- force-pushed the feature/ignore-property-exist-test-for-interfaces branch from 6332272 to 13eafa5 Compare August 2, 2018 15:00
@Nek-
Copy link
Contributor Author

Nek- commented Aug 20, 2018

It's up to date and issues of phpstan non-related.

@Nek- Nek- force-pushed the feature/ignore-property-exist-test-for-interfaces branch from 13eafa5 to ea36a5c Compare September 24, 2018 14:56
@soyuka
Copy link
Member

soyuka commented Feb 15, 2019

I think that #2495 fixes this case lmk if it doesn't.

Going to cherry pick the tests anyway!

/note for EU-FOSSA Hackathon just cherry-pick the test

@Nek-
Copy link
Contributor Author

Nek- commented Mar 13, 2019

@soyuka it does not. (I tried to add the test to the code base)

@Nek- Nek- force-pushed the feature/ignore-property-exist-test-for-interfaces branch from ea36a5c to 5155a3e Compare April 6, 2019 12:03
@Nek- Nek- changed the base branch from master to 2.4 April 6, 2019 12:03
@Nek- Nek- force-pushed the feature/ignore-property-exist-test-for-interfaces branch from 5155a3e to 05b3709 Compare April 6, 2019 12:19
@Nek- Nek- changed the title Add support for ignore property exist test for interface Ignore property that does not exist for interfaces Apr 6, 2019
@Nek- Nek- changed the title Ignore property that does not exist for interfaces Ignore property that does not exist for interfaces (identifiers for interfaces) Apr 6, 2019
Nek- added 2 commits April 6, 2019 14:59
1. it makes no sens to check if an interface exists in case it's an interface
2. If the property is not recognized as one and there is no identifier, then
   api platform will not be able to call the DataProvider for a given item
   and interface as a resource is an incomplete feature.
It should work like this but sadly it does not ATM. This is some WIP and
should not be tested for now.
@Nek- Nek- force-pushed the feature/ignore-property-exist-test-for-interfaces branch from 05b3709 to e05809e Compare April 6, 2019 12:59
@soyuka soyuka merged commit 5ef88f8 into api-platform:2.4 Apr 6, 2019
@Nek- Nek- deleted the feature/ignore-property-exist-test-for-interfaces branch April 6, 2019 14:42
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.

4 participants