Rework export by fitn · Pull Request #143 · FriendsOfAkeneo/CustomEntityBundle

fitn · 2018-01-23T14:44:53Z

Rework on the reference data export to handle linked objects

fitn · 2018-01-23T14:45:46Z

+    /**
+     * {@inheritdoc}
+     */
+    public function normalize($object, $format = null, array $context = []): array


I would have help to rework this part. Let me know what seems the better for you

Do you mean you would need help?

fitn · 2018-01-23T14:46:19Z

+ * @author    Romain Monceau <romain@akeneo.com>
+ * @copyright 2017 Akeneo SAS (http://www.akeneo.com)
+ */
+class ClassMetadataRegistry


Need help to find better naming, better namespace

Could you please add a description of this class. This will probably help with the naming 😉

Target entity resolver

fitn · 2018-01-23T14:46:34Z

+ * @author    Romain Monceau <romain@akeneo.com>
+ * @copyright 2017 Akeneo SAS (http://www.akeneo.com)
+ */
+class ReflectionClassRegistry


Need help to find better naming and better namespace

Same comment than previous class

ClassMetadataRegistry

jmleroux

Could you please add a description on this PR to help the review. And if you want, we can do a mob review IRL

jmleroux · 2018-01-23T16:24:24Z

+
+/**
+ * @author    Romain Monceau <romain@akeneo.com>
+ * @copyright 2017 Akeneo SAS (http://www.akeneo.com)


Please add a short description of this class features

jmleroux · 2018-01-23T16:24:58Z

+    /** @var NormalizerInterface */
+    protected $transNormalizer;
+
+    /** @var array */


@var string[]

jmleroux · 2018-01-23T22:28:51Z

+    /**
+     * {@inheritdoc}
+     */
+    public function normalize($object, $format = null, array $context = []): array


Do you mean you would need help?

jmleroux · 2018-01-23T22:29:08Z

+    {
+        $this->skippedFields = $skippedFields;
+    }
+}


jmleroux · 2018-01-23T22:30:02Z

+ * @author    Romain Monceau <romain@akeneo.com>
+ * @copyright 2017 Akeneo SAS (http://www.akeneo.com)
+ */
+class ClassMetadataRegistry


Could you please add a description of this class. This will probably help with the naming 😉

jmleroux · 2018-01-23T22:30:22Z

+ * @author    Romain Monceau <romain@akeneo.com>
+ * @copyright 2017 Akeneo SAS (http://www.akeneo.com)
+ */
+class ReflectionClassRegistry


Same comment than previous class

fitn · 2018-01-24T08:44:34Z

+    protected $classes = [];
+
+    /** @var array */
+    protected $properties = [];


readableProperties

fitn · 2018-01-24T14:30:25Z

+
+            $propertyValue = $this->propertyAccessor->getValue($object, $property);
+
+            if (is_object($propertyValue)) {


Extract in a submethod

fitn · 2018-01-24T14:30:43Z

+                        }
+
+                        $csvData[$property] = implode(',', $values);
+                    } elseif ($targetReflectionClass->implementsInterface(TranslationInterface::class)) {


Extract in dedicated ref data translation normalizer

fitn · 2018-01-24T14:31:14Z

@@ -0,0 +1,71 @@
+<?php
+
+namespace Pim\Bundle\CustomEntityBundle\Reflection;


fitn · 2018-01-24T14:31:43Z

+ * @author    Romain Monceau <romain@akeneo.com>
+ * @copyright 2017 Akeneo SAS (http://www.akeneo.com)
+ */
+class ClassMetadataRegistry


TargetEntityResolver

namespace Metadata

fitn · 2018-01-30T12:43:41Z

+        foreach ($properties as $property) {
+            if (in_array($property, $this->skippedFields)) {
+                continue;
+            }


Array_diff needs to be done before loop

mmetayer · 2018-01-30T13:18:55Z

+                }
+            }
+
+            $this->properties[$className] = $readableProperties;


$this->readableProperties[]

mmetayer · 2018-01-30T13:22:10Z

+            $this->properties[$className] = $readableProperties;
+        }
+
+        return $this->properties[$className];


return $this->readableProperties[$className];

mmetayer · 2018-01-30T13:24:53Z

+    }
+
+    /**
+     * @param mixed $object


Why mixed and not object?

mmetayer · 2018-01-30T13:27:28Z

+     * @param mixed $object
+     * @param string $property
+     *
+     * @return string


@return string|null

mmetayer · 2018-01-30T13:29:21Z

+    /**
+     * Normalized linked object property
+     *
+     * @param mixed $object


@param object $object ?

mmetayer · 2018-01-30T13:33:08Z

+    protected function normalizeLinkedObject($object, $property, $propertyValue, $format, $context)
+    {
+        $targetEntityClass = $this->targetEntityResolver->getTargetEntityClass($object, $property);
+        $targetReflectionClass = $this->classMetadataRegistry->get($targetEntityClass);


you should check if $targetEntityClass is null before doing that

Yep good catch. Done on the resolver side, it throws an exception if nothing found

mmetayer · 2018-01-30T13:39:15Z

+    }
+
+    /**
+     * Normalized linked object property


Normalize or Normalizes

jmleroux · 2018-01-24T08:33:35Z

+
+/**
+ * @author    Romain Monceau <romain@akeneo.com>
+ * @copyright 2017 Akeneo SAS (http://www.akeneo.com)


Please add a short description of this class features

jmleroux · 2018-01-24T08:39:27Z

+ * @author    Romain Monceau <romain@akeneo.com>
+ * @copyright 2017 Akeneo SAS (http://www.akeneo.com)
+ */
+class ClassMetadataRegistry


Target entity resolver

jmleroux · 2018-01-24T08:44:32Z

+    /** @var PropertyAccessorInterface */
+    protected $propertyAccessor;
+
+    /** @var array */


@var array cache for performance

jmleroux · 2018-01-24T08:45:03Z

+    /** @var array */
+    protected $classes = [];
+
+    /** @var array */


@var array cache for performance

jmleroux · 2018-01-24T08:55:23Z

+
+        $properties = $this->reflectionClassRegistry->getReadableProperties($object);
+        foreach ($properties as $property) {
+            if (in_array($property, $this->skippedFields)) {


use a filtered array instead of continue

fitn commented Jan 23, 2018

View reviewed changes

jmleroux requested changes Jan 23, 2018

View reviewed changes

jmleroux assigned fitn Jan 23, 2018

fitn commented Jan 24, 2018

View reviewed changes

fitn commented Jan 30, 2018

View reviewed changes

fitn force-pushed the export_refacto branch 2 times, most recently from 690df83 to fb5c7bd Compare January 30, 2018 13:08

mmetayer requested changes Jan 30, 2018

View reviewed changes

mmetayer approved these changes Jan 30, 2018

View reviewed changes

jmleroux approved these changes Jan 30, 2018

View reviewed changes

fitn mentioned this pull request Jan 31, 2018

GitHub 144 localizable custom entities #148

Open

fitn force-pushed the export_refacto branch from a1e4c6d to d79494f Compare February 2, 2018 14:11

Rework export

af18b4a

fitn force-pushed the export_refacto branch from d79494f to af18b4a Compare February 2, 2018 16:06

fitn merged commit ee814bd into master Feb 2, 2018

fitn deleted the export_refacto branch February 2, 2018 16:07


		$propertyValue = $this->propertyAccessor->getValue($object, $property);

		if (is_object($propertyValue)) {

		@@ -0,0 +1,71 @@
		<?php

		namespace Pim\Bundle\CustomEntityBundle\Reflection;

Uh oh!

Conversation

fitn commented Jan 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

fitn Jan 29, 2018 • 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!

jmleroux 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!

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!

mmetayer Jan 30, 2018 • 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!

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!

fitn commented Jan 23, 2018 •

edited

Loading

fitn Jan 29, 2018 •

edited

Loading

mmetayer Jan 30, 2018 •

edited

Loading