Support mapping properties in XML and YAML #814

meyerbaptiste · 2016-10-21T13:10:15Z

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#739, api-platform/api-platform#140
License	MIT
Doc PR	N/A

This PR adds support of mapping for properties in XML and YAML.
I followed the recommendations from #421.

soyuka

Nice job!!

soyuka · 2016-10-21T13:23:31Z

src/Metadata/Property/Factory/XmlPropertyMetadataFactory.php

+        $attributes = [];
+        foreach ($element->attribute as $attribute) {
+            $value = isset($attribute->attribute[0]) ? $this->getAttributes($attribute) : (string) $attribute;
+            isset($attribute['name']) ? $attributes[(string) $attribute['name']] = $value : $attributes[] = $value;


I find this hard to read, I'm not found of ternary value attributions, maybe it's just me

I find this hard to read too. I think an if-else is just fine here.

soyuka · 2016-10-21T13:26:35Z

src/Metadata/Property/Factory/YamlPropertyMetadataFactory.php

+                $parseException->setParsedFile($path);
+
+                throw $parseException;
+            }


nice one, maybe we should add this to the other Yaml factories!

teohhanhui · 2016-10-24T13:34:29Z

src/Metadata/Property/Factory/XmlPropertyMetadataFactory.php

+     */
+    private function update(PropertyMetadata $propertyMetadata, array $metadata) : PropertyMetadata
+    {
+        foreach (['description' => 'get', 'readable' => 'is', 'writable' => 'is', 'writableLink' => 'is', 'readableLink' => 'is', 'required' => 'is', 'identifier' => 'is', 'iri' => 'get', 'attributes' => 'get'] as $metadataKey => $accessorPrefix) {


Probably split the array into multiple lines.

dunglas · 2016-10-24T13:32:55Z

src/Metadata/Property/Factory/XmlPropertyMetadataFactory.php

+
+            return [
+                'description' => (string) $property['description'] ?: null,
+                'readable' => 'true' === (string) $property['readable'] || 'false' === (string) $property['readable'] ? constant((string) $property['readable']) : null,


Why not using XmlUtils::phpize instead of this trick?

Btw, (string) $property['readable'] can be '0' or '1' too according to the XSD spec.

dunglas · 2016-10-24T13:37:02Z

src/Metadata/Resource/Factory/XmlResourceMetadataFactory.php

-                    (string) $resource['shortName'] ?? null,
-                    (string) $resource['description'] ?? null,
-                    (string) $resource['iri'] ?? null,
+                    (string) $resource['shortName'] ?: null,


Why this change? What if the shortName key doesn't exist?

+1, anyway I thought this was tested but I've the feeling it is not and should be

Correct me if I am wrong but if an attribute does not exist in a \SimpleXMLElement class, its value is null so the result of this expression is never null (i.e. (string) null => '').

Example with $resource['foo'] which does not exist:

var_dump($resource); // class SimpleXMLElement var_dump($resource['foo']); // NULL var_dump((string) $resource['foo'] ?? null); // string(0) "" var_dump((string) $resource['foo'] ?: null); // NULL

I wasn't aware of this behavior, nice catch!

meyerbaptiste · 2016-10-26T13:18:57Z

Updated with your changes @soyuka @dunglas @teohhanhui.

Travis failed due to a depreciation notice from Twig (see twigphp/Twig#2208). This will be fixed by the next release of symfony/twig-bundle (i.e. 3.1.6).

dunglas · 2016-10-27T05:42:23Z

@meyerbaptiste can you use this trick twigphp/Twig#2208 (comment) to make Travis happy please?

meyerbaptiste · 2016-10-27T06:24:13Z

I tested it yesterday, Travis failed again with --prefer-lowest.

dunglas · 2016-10-27T06:26:18Z

You probably need to bump the minimal Twig version in composer.json to make Travis happy with --prefer-lowest.

meyerbaptiste · 2016-10-27T10:20:13Z

It's okay @dunglas, 3.1.6 is now released.

Simperfit · 2016-10-29T17:19:31Z

src/Metadata/Property/Factory/XmlPropertyMetadataFactory.php

+            return $this->handleNotFound($parentPropertyMetadata, $resourceClass, $property);
+        }
+
+        if ($parentPropertyMetadata) {


null !== $parentPropertyMetadata

Yes but #770 (comment).

Simperfit · 2016-10-29T17:20:27Z

src/Metadata/Property/Factory/XmlPropertyMetadataFactory.php

+     */
+    private function handleNotFound(PropertyMetadata $parentPropertyMetadata = null, string $resourceClass, string $property) : PropertyMetadata
+    {
+        if ($parentPropertyMetadata) {


null !== $parentPropertyMetadata

Simperfit · 2016-10-29T17:22:08Z

src/Metadata/Property/Factory/XmlPropertyMetadataFactory.php

+
+            $properties = (new \DOMXPath($domDocument))->query(sprintf('//resources/resource[@class="%s"]/property[@name="%s"]', $resourceClass, $propertyName));
+
+            if (false === $properties || 0 >= $properties->length || null === $properties->item(0) || false === $property = simplexml_import_dom($properties->item(0))) {


Could you change this for the sake of lisibility ?

if ( blabla || blabla)

Done @Simperfit.

dunglas · 2016-11-02T15:35:44Z

Thanks @meyerbaptiste. Don't forget to update the docs please.
Btw, it would be nice to add snippet of JS on the website hiding alternatives to annotations by default like on the Symfony's website.

…operty_configuration Support mapping properties in XML and YAML

meyerbaptiste force-pushed the add_xml_yml_property_configuration branch from 71a3006 to c528c92 Compare October 21, 2016 13:11

soyuka reviewed Oct 21, 2016

View reviewed changes

meyerbaptiste force-pushed the add_xml_yml_property_configuration branch from c528c92 to 5426fe1 Compare October 21, 2016 14:20

teohhanhui reviewed Oct 24, 2016

View reviewed changes

dunglas requested changes Oct 24, 2016

View reviewed changes

meyerbaptiste force-pushed the add_xml_yml_property_configuration branch 9 times, most recently from 65c5cfd to 865cb42 Compare October 26, 2016 13:04

meyerbaptiste force-pushed the add_xml_yml_property_configuration branch from 865cb42 to 1fede1a Compare October 27, 2016 10:08

Simperfit suggested changes Oct 29, 2016

View reviewed changes

Simperfit added the [RFR] Ready for Review label Oct 29, 2016

meyerbaptiste force-pushed the add_xml_yml_property_configuration branch 4 times, most recently from 37c48e1 to 1a6afd1 Compare October 31, 2016 10:18

Add support of mapping for properties in XML and YAML

ef5d037

meyerbaptiste force-pushed the add_xml_yml_property_configuration branch from 1a6afd1 to ef5d037 Compare October 31, 2016 10:35

Simperfit approved these changes Oct 31, 2016

View reviewed changes

dunglas merged commit 2a4bca8 into api-platform:master Nov 2, 2016

meyerbaptiste deleted the add_xml_yml_property_configuration branch November 2, 2016 15:36

Simperfit mentioned this pull request Nov 5, 2016

How do I configure which attribute is the hydra "@id" api-platform/api-platform#140

Closed

meyerbaptiste mentioned this pull request Nov 10, 2016

Refactor and merge YAML and XML loaders #837

Merged

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

Merge pull request api-platform#814 from meyerbaptiste/add_xml_yml_pr…

34991ef

…operty_configuration Support mapping properties in XML and YAML


		$properties = (new \DOMXPath($domDocument))->query(sprintf('//resources/resource[@class="%s"]/property[@name="%s"]', $resourceClass, $propertyName));

		if (false === $properties \|\| 0 >= $properties->length \|\| null === $properties->item(0) \|\| false === $property = simplexml_import_dom($properties->item(0))) {

Uh oh!

Support mapping properties in XML and YAML #814

Support mapping properties in XML and YAML #814

Uh oh!

Conversation

meyerbaptiste commented Oct 21, 2016

Uh oh!

soyuka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

meyerbaptiste commented Oct 26, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dunglas commented Oct 27, 2016

Uh oh!

meyerbaptiste commented Oct 27, 2016

Uh oh!

dunglas commented Oct 27, 2016

Uh oh!

meyerbaptiste commented Oct 27, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Simperfit Oct 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dunglas commented Nov 2, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

meyerbaptiste commented Oct 26, 2016 •

edited

Loading

Simperfit Oct 29, 2016 •

edited

Loading

dunglas commented Nov 2, 2016 •

edited

Loading