File resource configuration (xml,yml) #490

soyuka · 2016-04-04T10:28:15Z

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	no
Fixed tickets	#421
License	MIT
Doc PR	api-platform/docs#42

TODO:

fix custom operations (Notice: Undefined variable: actionName #491)
add behat test on configuration-declared resource

teohhanhui · 2016-04-05T12:21:31Z

src/Bridge/Symfony/Bundle/DependencyInjection/ApiPlatformExtension.php

@@ -137,6 +138,35 @@ private function registerAnnotationLoaders(ContainerBuilder $container)
    }

    /**
+     * Registers configuration loaders.


configuration file loaders

soyuka · 2016-04-05T14:42:23Z

src/Metadata/Resource/Factory/XmlResourceMetadataFactory.php

+            $metadata['shortName'] = $reflectionClass->name;
+        }
+
+        $resource = new Resource();


I'm re-using the ApiPlatform\Core\Annotation\Resource class. Maybe we should consider renaming it?

We still need the annotation class for annotations.

Why do we need to use this as an intermediate, instead of calling the with... methods of ResourceMetadata directly?

ofc

To avoid code repetition in {Annotation,Xml,Yaml}ResourceMetadataFactory? AnnotationResourceMetadataFactory is using an Object to build metadata. Btw I reverted the ApiPlatform\Core\Annotation\Resource file to the original value.

soyuka · 2016-04-05T14:54:33Z

tests/Metadata/Resource/Factory/FileLoaderResourceNameCollectionFactoryTest.php

+        $resourceMetadata = $resourceMetadataFactory->create(ConfigDummy::class);
+
+        $this->assertInstanceOf(ResourceMetadata::class, $resourceMetadata);
+    }


@teohhanhui for example.

Now I need to see if createMetadata should be kept in the ResourceMetadataFactoryTrait, which depends on the use of Annotation/Resource as a pre-metadata class (see 9463498#r58549969).

soyuka · 2016-04-05T15:18:00Z

src/Metadata/Resource/Factory/ResourceMetadataFactoryTrait.php

+        }
+
+        $operations = [];
+        //@TODO - Parse annotations/configuration and build OperationResourceMetadataFactory manually


Here, I'd love to see an Operation merge instead of replacing defaults. Dunno if it's too much or maybe wrong but I'd allow less configuration for adding a custom operation.

@see OperationResourceMetadataFactory

teohhanhui · 2016-04-05T15:35:11Z

src/Metadata/Resource/Factory/ResourceMetadataFactoryTrait.php

+use ApiPlatform\Core\Metadata\Resource\ResourceMetadata;
+use ApiPlatform\Core\Annotation\Resource;
+
+trait ResourceMetadataFactoryTrait


I'm 👎 for this trait due to violation of SRP (single responsibility principle).

In any case, things are most likely to change once you're done rewriting for the current codebase.

This would indeed resolve the issue involving the use of Annotation\Resource with configuration values.
Still, it's handy for the handleNotFound method.

dunglas · 2016-04-25T17:58:22Z

src/Metadata/Resource/Factory/XmlResourceMetadataFactory.php

+        foreach ($this->paths as $path) {
+            $this->xmlParser->loadXML(file_get_contents($path));
+
+            $xpath = new DOMXpath($this->xmlParser);


We should create a XML schema to validate the config provided by the user: http://php.net/manual/fr/domdocument.schemavalidate.php

dunglas · 2016-04-25T18:09:29Z

Amazing work @soyuka! Thank you very much.

I left some comment but it's very important improvement to API Platform.

soyuka · 2016-04-29T17:05:52Z

Just so you know, I won't be available until Mar. 8 and I hope I'll have time to update this before the 20th.

soyuka · 2016-05-10T21:02:59Z

src/Metadata/Resource/Factory/XmlResourceMetadataFactory.php

+
+            if (false === @$this->xmlParser->schemaValidate(self::RESOURCE_SCHEMA)) {
+                throw new \InvalidArgumentException(sprintf('XML Schema loaded from path %s is not valid!', realpath($path)));
+            }


I'm not sure about this, not very fond of preventing the Warning.

As far as I can tell, Symfony does it everywhere too:

https://github.com/symfony/symfony/blob/af9ec74edb2498dc9eaf175e7e62815a06c8d6ba/src/Symfony/Component/DependencyInjection/Loader/XmlFileLoader.php#L487

https://github.com/symfony/symfony/blob/f29d46f29b91ea5c30699cf6bdb8e65545d1dd26/src/Symfony/Component/Translation/Loader/XliffFileLoader.php#L173

It looks good to me.

dunglas · 2016-05-11T06:43:21Z

src/Bridge/Symfony/Bundle/Resources/schema/xml_metadata.xsd

@@ -0,0 +1,73 @@
+<xs:schema attributeFormDefault="unqualified" elementFormDefault="qualified" xmlns:xs="http://www.w3.org/2001/XMLSchema">


The XSD isn't related with Symfony (it will be useful if you use the standalone library too).

I suggest src/schema/metadata.xsd

dunglas · 2016-05-11T13:45:23Z

It looks good to me, very helpful work @soyuka, thanks! Do you think this PR is ready to be merged now?

soyuka · 2016-05-11T14:00:04Z

Yes @dunglas It looks good to me!

dunglas · 2016-05-11T15:20:09Z

Thank you!

dunglas · 2016-05-11T15:46:42Z

@soyuka this PR introduced some Scrutinizr issues (problems are mostly related to the complexity of the XML Loader). Can you take a look? https://scrutinizer-ci.com/g/api-platform/core/inspections/8232a220-c47f-469d-bc1e-81d9716d1e60/code-structure/

soyuka · 2016-05-11T15:56:31Z

@dunglas Yes I've worked around some improvements before my last commit.

About code repetitions, I could avoid them by using a Trait that shares common methods. But, we discussed this above with @teohhanhui. I could however add a Trait for Xml metadata that shares validation/error printing methods etc.

About things like The method getAttribute cannot be called on $metadata (of type null). or The variable $xpath does not seem to be defined for all execution paths leading up to this point. (which shows up as "complexity", to me it's more an issue about scrutinizer not checking the loop-declared variables. I want the code to behave like this, but I don't see how it adds any complexity.

I'm willing to do another PR with small fixes no problem ;). Do you already have a release date in mind for the v2?

teohhanhui · 2016-05-12T05:51:27Z

Thank you @soyuka for seeing this through. I'm going to give this a try in my project...

teohhanhui · 2016-05-12T05:57:14Z

Uhh, ideally I'd want separate configuration files for each resource. Maybe I'll look into contributing that...

Also, the name resources.{xml,yml} is problematic. We should at least make it api_resources.{xml,yml}

File resource configuration (xml,yml)

soyuka force-pushed the fileloader branch 3 times, most recently from 0bc3370 to a02e71f Compare April 4, 2016 14:27

soyuka changed the title ~~(WIP) File resource configuration (xml,yml)~~ [Wip] File resource configuration (xml,yml) Apr 4, 2016

soyuka changed the title ~~[Wip] File resource configuration (xml,yml)~~ [WIP] File resource configuration (xml,yml) Apr 4, 2016

soyuka force-pushed the fileloader branch 5 times, most recently from f6012ce to 37f5832 Compare April 5, 2016 10:15

teohhanhui reviewed Apr 5, 2016
View reviewed changes

soyuka force-pushed the fileloader branch 3 times, most recently from 021e607 to 7f484e8 Compare April 5, 2016 14:42

soyuka reviewed Apr 5, 2016
View reviewed changes

soyuka force-pushed the fileloader branch from 7f484e8 to 9463498 Compare April 5, 2016 14:43

soyuka reviewed Apr 5, 2016
View reviewed changes

soyuka force-pushed the fileloader branch from 9463498 to 828dda7 Compare April 5, 2016 15:16

soyuka reviewed Apr 5, 2016
View reviewed changes

soyuka force-pushed the fileloader branch 4 times, most recently from c340705 to c8e4f11 Compare April 5, 2016 15:34

teohhanhui reviewed Apr 5, 2016
View reviewed changes

dunglas reviewed Apr 25, 2016
View reviewed changes

soyuka force-pushed the fileloader branch from 9b65547 to 4eb6e23 Compare May 10, 2016 21:01

soyuka reviewed May 10, 2016
View reviewed changes

soyuka force-pushed the fileloader branch 2 times, most recently from 118535a to 2ce7d5e Compare May 11, 2016 06:13

dunglas reviewed May 11, 2016
View reviewed changes

soyuka force-pushed the fileloader branch 3 times, most recently from e24ef7e to 6fada9d Compare May 11, 2016 08:11

File resource configuration (xml,yml)

cacc5ec

soyuka force-pushed the fileloader branch from 6fada9d to cacc5ec Compare May 11, 2016 08:37

dunglas merged commit 3a6d5c7 into api-platform:master May 11, 2016

teohhanhui mentioned this pull request Jun 3, 2016

v2 metadata - YAML resource config #421

Closed

soyuka deleted the fileloader branch September 9, 2016 16:01

magarzon pushed a commit to magarzon/core that referenced this pull request Feb 12, 2017

Merge pull request api-platform#490 from soyuka/fileloader

f4a0fd3

File resource configuration (xml,yml)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

File resource configuration (xml,yml) #490

File resource configuration (xml,yml) #490

soyuka commented Apr 4, 2016 •

edited

Loading

teohhanhui Apr 5, 2016

soyuka Apr 5, 2016

teohhanhui Apr 5, 2016

soyuka Apr 5, 2016

soyuka Apr 5, 2016

soyuka Apr 5, 2016

teohhanhui Apr 5, 2016

soyuka Apr 5, 2016

dunglas Apr 25, 2016

dunglas commented Apr 25, 2016

soyuka commented Apr 29, 2016

soyuka May 10, 2016

teohhanhui May 11, 2016

dunglas May 11, 2016

dunglas May 11, 2016

dunglas commented May 11, 2016

soyuka commented May 11, 2016 •

edited

Loading

dunglas commented May 11, 2016

dunglas commented May 11, 2016

soyuka commented May 11, 2016 •

edited

Loading

teohhanhui commented May 12, 2016

teohhanhui commented May 12, 2016

		@@ -0,0 +1,73 @@
		<xs:schema attributeFormDefault="unqualified" elementFormDefault="qualified" xmlns:xs="http://www.w3.org/2001/XMLSchema">

File resource configuration (xml,yml) #490

File resource configuration (xml,yml) #490

Conversation

soyuka commented Apr 4, 2016 • 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

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

dunglas commented Apr 25, 2016

soyuka commented Apr 29, 2016

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

dunglas commented May 11, 2016

soyuka commented May 11, 2016 • edited Loading

dunglas commented May 11, 2016

dunglas commented May 11, 2016

soyuka commented May 11, 2016 • edited Loading

teohhanhui commented May 12, 2016

teohhanhui commented May 12, 2016

soyuka commented Apr 4, 2016 •

edited

Loading

soyuka commented May 11, 2016 •

edited

Loading

soyuka commented May 11, 2016 •

edited

Loading