Skip to content

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

Merged
soyuka merged 2 commits into
api-platform:2.4from
Nek-:feature/ignore-property-exist-test-for-interfaces
Apr 6, 2019
Merged

Ignore property that does not exist for interfaces (identifiers for interfaces)#2086
soyuka merged 2 commits into
api-platform:2.4from
Nek-:feature/ignore-property-exist-test-for-interfaces

Conversation

@Nek-

@Nek- Nek- commented Jul 10, 2018

Copy link
Copy Markdown
Contributor
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
Comment thread src/Metadata/Property/Factory/ExtractorPropertyMetadataFactory.php
@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
Copy Markdown
Contributor

I thought we already support virtual properties?

@Nek-

Nek- commented Jul 16, 2018

Copy link
Copy Markdown
Contributor Author

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

@teohhanhui

Copy link
Copy Markdown
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-

Nek- commented Jul 16, 2018

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
Contributor

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

Comment thread tests/Fixtures/FileConfigurations/interface_resource.yml
@Nek-
Nek- force-pushed the feature/ignore-property-exist-test-for-interfaces branch from 6332272 to 13eafa5 Compare August 2, 2018 15:00
@Nek-

Nek- commented Aug 20, 2018

Copy link
Copy Markdown
Contributor Author

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

Comment thread src/Metadata/Property/Factory/ExtractorPropertyMetadataFactory.php Outdated
@Nek-
Nek- force-pushed the feature/ignore-property-exist-test-for-interfaces branch from 13eafa5 to ea36a5c Compare September 24, 2018 14:56
@soyuka

soyuka commented Feb 15, 2019

Copy link
Copy Markdown
Member

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-

Nek- commented Mar 13, 2019

Copy link
Copy Markdown
Contributor Author

@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
Comment thread src/Metadata/Property/Factory/ExtractorPropertyMetadataFactory.php Outdated
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