[API] allow extending and overwrite api in yaml + test #12693

SirDomin · 2021-06-07T07:49:01Z

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Related tickets
License	MIT

src/Sylius/Bundle/ApiBundle/ApiPlatform/Factory/MergingExtractorResourceMetadataFactory.php

soyuka · 2021-06-08T12:02:23Z

src/Sylius/Bundle/ApiBundle/test/config/api_platform/config.yaml

+'%sylius.model.zone.class%':
+    collectionOperations:
+        admin_get (unset): ~
+        admin_post (unset): ~


👎 operations should not be merged but replaced and it'd solve everyone's issues.

Do you mean, that we should replace the whole configuration of the resource? That would make sense. But the issue is, that for many cases Order would be replaced with the new configuration and once the configuration is replaced you will never receive upstream changes or features - therefore BC may be challenging on our side and it may raise some issues on the user side to determine what has changed in core and apply changes to his/hers app. The problem would be smaller if we would be able to declare more granular resources. It would be perfect if one entity could be represented by several resources. For example, one order resource could be represented as shop order, admin order, and cart. Once we would reduce size of configuration files replacement seems to be much more viable option

Yes really interesting, I hope that the next version of API Platform will help you to do so in an less constricting way! Btw are you doing Archtectural Decision Records for these kind of things?

For sure we should add for this 👍

src/Sylius/Bundle/ApiBundle/ApiPlatform/Factory/MergingExtractorResourceMetadataFactory.php

soyuka · 2021-06-08T12:07:00Z

src/Sylius/Bundle/ApiBundle/ApiPlatform/Factory/MergingExtractorResourceMetadataFactory.php

+        $resultingConfig = [];
+
+        foreach ($configs as $config) {
+            foreach ($config as $newKey => $newValue) {


I really think that this is a bad idea, replacing the configuration over merging would be easier and more obvious.

Zales0123

As it's quite a complicated and potentially an error-prone feature, maybe it would be good to add at least one more test to unset some custom operation, something like shop_select_shipping_method? I would personally feel more secured if we also check this feature on a non-crud operation 🖖

src/Sylius/Bundle/ApiBundle/test/src/Tests/SyliusRouteDisableTest.php

src/Sylius/Bundle/ApiBundle/ApiPlatform/Factory/MergingExtractorResourceMetadataFactory.php

GSadee

For me, there are some missing tests for cases like overwriting the existing endpoint by changing it (not only unsetting) or checking that endpoints from the base configuration are not replaced by for example adding a new one.

adr/2021_06_15_api_platform_config_customization.md

src/Sylius/Bundle/ApiBundle/ApiPlatform/ConfigMergeManager.php

src/Sylius/Bundle/ApiBundle/ApiPlatform/Factory/MergingExtractorResourceMetadataFactory.php

src/Sylius/Bundle/ApiBundle/ApiPlatform/ResourceMetadataPropertyValueResolver.php

adr/2021_06_15_api_platform_config_customization.md

src/Sylius/Bundle/ApiBundle/ApiPlatform/Factory/MergingExtractorResourceMetadataFactory.php

src/Sylius/Bundle/ApiBundle/Resources/config/services.xml

src/Sylius/Bundle/ApiBundle/ApiPlatform/Factory/MergingExtractorResourceMetadataFactory.php

adr/2021_06_15_api_platform_config_customization.md

src/Sylius/Bundle/ApiBundle/ApiPlatform/ConfigMergeManager.php

src/Sylius/Bundle/ApiBundle/ApiPlatform/ResourceMetadataPropertyValueResolver.php

src/Sylius/Bundle/ApiBundle/spec/ApiPlatform/ConfigMergeManagerSpec.php

lchrusciel · 2021-06-18T17:25:15Z

src/Sylius/Bundle/ApiBundle/ApiPlatform/ApiResourceConfigurationMerger.php

+namespace Sylius\Bundle\ApiBundle\ApiPlatform;
+
+/** @experimental */
+class ApiResourceConfigurationMerger implements ApiResourceConfigurationMergerInterface


Suggested change

class ApiResourceConfigurationMerger implements ApiResourceConfigurationMergerInterface

final class ApiResourceConfigurationMerger implements ApiResourceConfigurationMergerInterface

im unable to write tests for it when its final (same with ResourceMetadataPropertyValueResolver)

You should use interface in specs

lchrusciel · 2021-06-18T17:25:34Z

src/Sylius/Bundle/ApiBundle/ApiPlatform/ResourceMetadataPropertyValueResolver.php

+use ApiPlatform\Core\Metadata\Resource\ResourceMetadata;
+
+/** @experimental */
+class ResourceMetadataPropertyValueResolver implements ResourceMetadataPropertyValueResolverInteface


Suggested change

class ResourceMetadataPropertyValueResolver implements ResourceMetadataPropertyValueResolverInteface

final class ResourceMetadataPropertyValueResolver implements ResourceMetadataPropertyValueResolverInteface

src/Sylius/Bundle/ApiBundle/ApiPlatform/Factory/MergingExtractorResourceMetadataFactory.php

src/Sylius/Bundle/ApiBundle/spec/ApiPlatform/ResourceMetadataPropertyValueResolverSpec.php

arti0090 · 2021-06-22T07:02:31Z

src/Sylius/Bundle/ApiBundle/Resources/config/services.xml

+
+        <service id="Sylius\Bundle\ApiBundle\ApiPlatform\ApiResourceConfigurationMerger" />
+
+        <service id="Sylius\Bundle\ApiBundle\ApiPlatform\ResourceMetadataPropertyValueResolver" >


Suggested change

<service id="Sylius\Bundle\ApiBundle\ApiPlatform\ResourceMetadataPropertyValueResolver" >

<service id="Sylius\Bundle\ApiBundle\ApiPlatform\ResourceMetadataPropertyValueResolver">

GSadee · 2021-06-22T07:39:43Z

src/Sylius/Bundle/ApiBundle/spec/ApiPlatform/ResourceMetadataPropertyValueResolverSpec.php

+        $this->beConstructedWith($apiResourceConfigurationMerger);
+    }
+
+    function it_returns_merged_configs(ApiResourceConfigurationMerger $apiResourceConfigurationMerger): void


Suggested change

function it_returns_merged_configs(ApiResourceConfigurationMerger $apiResourceConfigurationMerger): void

function it_returns_merged_configs(ApiResourceConfigurationMergerInterface $apiResourceConfigurationMerger): void

GSadee · 2021-06-22T07:40:10Z

src/Sylius/Bundle/ApiBundle/ApiPlatform/ApiResourceConfigurationMerger.php

+namespace Sylius\Bundle\ApiBundle\ApiPlatform;
+
+/** @experimental */
+class ApiResourceConfigurationMerger implements ApiResourceConfigurationMergerInterface


You should use interface in specs

GSadee · 2021-06-22T07:43:00Z

Thank you, @SirDomin! 🥇

This PR was merged into the 1.11-dev branch. Discussion ---------- | Q | A | --------------- | ----- | Branch? | master | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Related tickets | docs for #12693 | License | MIT  Commits ------- f59c7cc add docs about yaml 0f9e0f8 pr-fix 36ec10f add info about overwriting operation

This PR was merged into the 1.11-dev branch. Discussion ---------- | Q | A | --------------- | ----- | Branch? | master | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Related tickets | #12693 | License | MIT  Commits ------- 285edea add final to clssess, cs fixes

This PR was merged into the 1.11-dev branch. Discussion ---------- | Q | A | --------------- | ----- | Branch? | master | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Related tickets | Sylius/Sylius#12693 | License | MIT  Commits ------- 285edea63378e3a8746e710f05927ec4ad461e34 add final to clssess, cs fixes

This PR was merged into the 1.11 branch. Discussion ---------- | Q | A | |-----------------|--------------------------------------------------------------| | Branch? | 1.11 | | Bug fix? | yes | | New feature? | no | | BC breaks? | no | | Deprecations? | no | | Related tickets | fixes #14268 | | License | MIT | This PR removes the `%kernel.project_dir%/config/api_platform` mapping path that was introduced in #12693. In API Platform 2.6 users' mapping configuration is overwritten with bundles' mapping. Because of it, developers cannot override resources in `config/api_platform` as Sylius overrides users' resources placed in `config/api_platform`. It's already fixed in APIP 2.7 but for now, this configuration doesn't work as intended. We haven't noticed in unit tests that it breaks resource overriding because we have a distinction between `config/api_platform` and `config/api_resource`, where `api_resource` can override resources while `api_platform` can't. This distinction is something reasonable but for testing purposes, we should keep everything in one directory because it makes possible to test this bug. Commits ------- fe1d660 [API] Remove mapping path from configuration

SirDomin requested a review from a team as a code owner June 7, 2021 07:49

probot-autolabeler bot added the API APIs related issues and PRs. label Jun 7, 2021

SirDomin changed the title ~~[API] allow removing routes in yaml + test~~ [API] allow extending and overwrite api in yaml + test Jun 7, 2021

SirDomin force-pushed the sylius-api-yaml-support branch 15 times, most recently from cbfb8d9 to 4a7264b Compare June 8, 2021 07:38

AdamKasp reviewed Jun 8, 2021

View reviewed changes

soyuka reviewed Jun 8, 2021

View reviewed changes

SirDomin mentioned this pull request Jun 8, 2021

[Docs] Docs customization #12700

Merged

Zales0123 reviewed Jun 10, 2021

View reviewed changes

src/Sylius/Bundle/ApiBundle/test/src/Tests/SyliusRouteDisableTest.php Outdated Show resolved Hide resolved

lchrusciel reviewed Jun 10, 2021

View reviewed changes

src/Sylius/Bundle/ApiBundle/ApiPlatform/Factory/MergingExtractorResourceMetadataFactory.php Outdated Show resolved Hide resolved

src/Sylius/Bundle/ApiBundle/ApiPlatform/Factory/MergingExtractorResourceMetadataFactory.php Outdated Show resolved Hide resolved

SirDomin force-pushed the sylius-api-yaml-support branch 7 times, most recently from 3a54281 to e933156 Compare June 10, 2021 12:06

SirDomin force-pushed the sylius-api-yaml-support branch from e933156 to d2b7f0d Compare June 10, 2021 12:10

GSadee requested changes Jun 11, 2021

View reviewed changes

SirDomin added 4 commits June 14, 2021 09:44

added more test cases

bc2249d

refactor + alot of specs

acd6f88

fix error message handling

004188b

added adr

e48d14e

probot-autolabeler bot added the Maintenance CI configurations, READMEs, releases, etc. label Jun 15, 2021

AdamKasp reviewed Jun 15, 2021

View reviewed changes

pr-fix

384f99d

SirDomin force-pushed the sylius-api-yaml-support branch from 7f020cb to 384f99d Compare June 15, 2021 13:14

arti0090 reviewed Jun 16, 2021

View reviewed changes

GSadee approved these changes Jun 16, 2021

View reviewed changes

src/Sylius/Bundle/ApiBundle/ApiPlatform/Factory/MergingExtractorResourceMetadataFactory.php Outdated Show resolved Hide resolved

Coding standards fixes

d93a6fb

lchrusciel reviewed Jun 16, 2021

View reviewed changes

SirDomin added 2 commits June 16, 2021 23:21

pr-fix, refactor

f90c20c

move finding unset to private function

eb20453

lchrusciel approved these changes Jun 18, 2021

View reviewed changes

Zales0123 approved these changes Jun 21, 2021

View reviewed changes

coding-standards fixed

715377a

alanpoulain mentioned this pull request Jun 21, 2021

[RFC] Resource configuration inheritance (Sylius) api-platform/core#4091

Closed

arti0090 approved these changes Jun 22, 2021

View reviewed changes

GSadee reviewed Jun 22, 2021

View reviewed changes

GSadee merged commit 86b80ff into Sylius:master Jun 22, 2021

SirDomin mentioned this pull request Jun 22, 2021

[API] add final to clssess, cs fixes #12743

Merged

coldic3 mentioned this pull request Aug 30, 2022

[API] Remove problematic mapping path #14236

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[API] allow extending and overwrite api in yaml + test #12693

[API] allow extending and overwrite api in yaml + test #12693

SirDomin commented Jun 7, 2021

soyuka Jun 8, 2021

lchrusciel Jun 9, 2021 •

edited

Loading

soyuka Jun 10, 2021

lchrusciel Jun 10, 2021

soyuka Jun 8, 2021

Zales0123 left a comment

GSadee left a comment

lchrusciel Jun 18, 2021

SirDomin Jun 22, 2021

GSadee Jun 22, 2021

lchrusciel Jun 18, 2021

arti0090 Jun 22, 2021

GSadee Jun 22, 2021

GSadee Jun 22, 2021

GSadee commented Jun 22, 2021

	class ApiResourceConfigurationMerger implements ApiResourceConfigurationMergerInterface
	final class ApiResourceConfigurationMerger implements ApiResourceConfigurationMergerInterface

	class ResourceMetadataPropertyValueResolver implements ResourceMetadataPropertyValueResolverInteface
	final class ResourceMetadataPropertyValueResolver implements ResourceMetadataPropertyValueResolverInteface


		<service id="Sylius\Bundle\ApiBundle\ApiPlatform\ApiResourceConfigurationMerger" />

		<service id="Sylius\Bundle\ApiBundle\ApiPlatform\ResourceMetadataPropertyValueResolver" >

	function it_returns_merged_configs(ApiResourceConfigurationMerger $apiResourceConfigurationMerger): void
	function it_returns_merged_configs(ApiResourceConfigurationMergerInterface $apiResourceConfigurationMerger): void

[API] allow extending and overwrite api in yaml + test #12693

[API] allow extending and overwrite api in yaml + test #12693

Conversation

SirDomin commented Jun 7, 2021

Choose a reason for hiding this comment

lchrusciel Jun 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zales0123 left a comment

Choose a reason for hiding this comment

GSadee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GSadee commented Jun 22, 2021

lchrusciel Jun 9, 2021 •

edited

Loading