Handle field name, no array default sort, enforce type for serializer…

… hydration, prevent regression in PostMountEvent constructor
symfony · Dec 1, 2023 · e700c92 · e700c92
1 parent 8c2155c
commit e700c92
Show file tree

Hide file tree

Showing 17 changed files with 134 additions and 80 deletions.
diff --git a/src/LiveComponent/src/Attribute/LiveProp.php b/src/LiveComponent/src/Attribute/LiveProp.php
@@ -63,7 +63,7 @@ final class LiveProp
      *
      * Tells if this property should be bound to the URL
      */
-    private bool $url;
+    private bool $queryMapping;
 
     /**
      * @param bool|array  $writable                  If true, this property can be changed by the frontend.
@@ -80,7 +80,7 @@ final class LiveProp
      *                                               from the value used when originally rendering
      *                                               this child, the value in the child will be updated
      *                                               to match the new value and the child will be re-rendered
-     * @param bool        $url                       if true, this property will be synchronized with a query parameter
+     * @param bool        $queryMapping              if true, this property will be synchronized with a query parameter
      *                                               in the URL
      */
     public function __construct(
@@ -93,7 +93,7 @@ public function __construct(
         string $format = null,
         bool $updateFromParent = false,
         string|array $onUpdated = null,
-        bool $url = false,
+        bool $queryMapping = false,
     ) {
         $this->writable = $writable;
         $this->hydrateWith = $hydrateWith;
@@ -104,7 +104,7 @@ public function __construct(
         $this->format = $format;
         $this->acceptUpdatesFromParent = $updateFromParent;
         $this->onUpdated = $onUpdated;
-        $this->url = $url;
+        $this->queryMapping = $queryMapping;
 
         if ($this->useSerializerForHydration && ($this->hydrateWith || $this->dehydrateWith)) {
             throw new \InvalidArgumentException('Cannot use useSerializerForHydration with hydrateWith or dehydrateWith.');
@@ -200,8 +200,8 @@ public function onUpdated(): null|string|array
         return $this->onUpdated;
     }
 
-    public function url(): bool
+    public function queryMapping(): bool
     {
-        return $this->url;
+        return $this->queryMapping;
     }
 }
diff --git a/src/LiveComponent/src/EventListener/QueryStringInitializeSubscriber.php b/src/LiveComponent/src/EventListener/QueryStringInitializeSubscriber.php
@@ -43,6 +43,11 @@ public static function getSubscribedEvents(): array
 
     public function onPreMount(PreMountEvent $event): void
     {
+        if (!$event->getMetadata()->get('live', false)) {
+            // Not a live component
+            return;
+        }
+
         $request = $this->requestStack->getMainRequest();
 
         if (null === $request) {

diff --git a/src/LiveComponent/src/LiveComponentHydrator.php b/src/LiveComponent/src/LiveComponentHydrator.php
@@ -232,6 +232,8 @@ public function hydrate(object $component, array $props, array $updatedProps, Li
      *
      * Depending on the prop configuration, the value may be hydrated by a custom method or the Serializer component.
      *
+     * @internal
+     *
      * @throws SerializerExceptionInterface
      */
     public function hydrateValue(mixed $value, LivePropMetadata $propMetadata, object $parentObject): mixed
@@ -245,6 +247,10 @@ public function hydrateValue(mixed $value, LivePropMetadata $propMetadata, objec
         }
 
         if ($propMetadata->useSerializerForHydration()) {
+            if (null === $propMetadata->getType()) {
+                throw new \LogicException(sprintf('The "%s::%s" object should be hydrated with the Serializer, but no type could be guessed.', $parentObject::class, $propMetadata->getName()));
+            }
+
             return $this->normalizer->denormalize($value, $propMetadata->getType(), 'json', $propMetadata->serializationContext());
         }
 

diff --git a/src/LiveComponent/src/Metadata/LiveComponentMetadata.php b/src/LiveComponent/src/Metadata/LiveComponentMetadata.php
@@ -65,7 +65,7 @@ public function getOnlyPropsThatAcceptUpdatesFromParent(array $inputProps): arra
     public function hasQueryStringBindings(): bool
     {
         foreach ($this->getAllLivePropsMetadata() as $livePropMetadata) {
-            if ([] !== $livePropMetadata->getQueryStringMapping()) {
+            if ($livePropMetadata->queryStringMapping()) {
                 return true;
             }
         }

diff --git a/src/LiveComponent/src/Metadata/LiveComponentMetadataFactory.php b/src/LiveComponent/src/Metadata/LiveComponentMetadataFactory.php
@@ -103,16 +103,14 @@ public function createLivePropMetadata(string $className, string $propertyName,
             $isTypeNullable = $type?->allowsNull() ?? true;
         }
 
-        $queryStringBinding = $this->createQueryStringMapping($propertyName, $liveProp);
-
         return new LivePropMetadata(
             $property->getName(),
             $liveProp,
             $infoType,
             $isTypeBuiltIn,
             $isTypeNullable,
             $collectionValueType,
-            $queryStringBinding
+            $liveProp->queryMapping()
         );
     }
 
@@ -130,17 +128,6 @@ private static function propertiesFor(\ReflectionClass $class): iterable
         }
     }
 
-    private function createQueryStringMapping(string $propertyName, LiveProp $liveProp): array
-    {
-        if (false === $liveProp->url()) {
-            return [];
-        }
-
-        return [
-            'name' => $propertyName,
-        ];
-    }
-
     public function reset(): void
     {
         $this->liveComponentMetadata = [];

diff --git a/src/LiveComponent/src/Metadata/LivePropMetadata.php b/src/LiveComponent/src/Metadata/LivePropMetadata.php
@@ -30,7 +30,7 @@ public function __construct(
         private bool $isBuiltIn,
         private bool $allowsNull,
         private ?Type $collectionValueType,
-        private array $queryStringMapping = [],
+        private bool $queryStringMapping,
     ) {
     }
 
@@ -54,10 +54,7 @@ public function allowsNull(): bool
         return $this->allowsNull;
     }
 
-    /**
-     * @return array{'name': string}
-     */
-    public function getQueryStringMapping(): array
+    public function queryStringMapping(): bool
     {
         return $this->queryStringMapping;
     }

diff --git a/src/LiveComponent/src/Util/LiveControllerAttributesCreator.php b/src/LiveComponent/src/Util/LiveControllerAttributesCreator.php
@@ -98,28 +98,29 @@ public function attributesForRendering(MountedComponent $mounted, ComponentMetad
             $mountedAttributes = $mountedAttributes->defaults(['data-live-id' => $id]);
         }
 
-        if ($isChildComponent) {
-            $fingerprint = $this->fingerprintCalculator->calculateFingerprint(
-                $mounted->getInputProps(),
-                $this->metadataFactory->getMetadata($mounted->getName())
-            );
-            if ($fingerprint) {
-                $attributesCollection->setFingerprint($fingerprint);
-            }
-        }
-
         $liveMetadata = $this->metadataFactory->getMetadata($mounted->getName());
 
         if ($liveMetadata->hasQueryStringBindings()) {
             $queryMapping = [];
             foreach ($liveMetadata->getAllLivePropsMetadata() as $livePropMetadata) {
-                if ($mapping = $livePropMetadata->getQueryStringMapping()) {
-                    $queryMapping[$livePropMetadata->getName()] = $mapping;
+                if ($livePropMetadata->queryStringMapping()) {
+                    $frontendName = $livePropMetadata->calculateFieldName($mounted, $livePropMetadata->getName());
+                    $queryMapping[$frontendName] = ['name' => $frontendName];
                 }
             }
             $attributesCollection->setQueryUrlMapping($queryMapping);
         }
 
+        if ($isChildComponent) {
+            $fingerprint = $this->fingerprintCalculator->calculateFingerprint(
+                $mounted->getInputProps(),
+                $liveMetadata
+            );
+            if ($fingerprint) {
+                $attributesCollection->setFingerprint($fingerprint);
+            }
+        }
+
         $dehydratedProps = $this->dehydrateComponent(
             $mounted->getName(),
             $mounted->getComponent(),

diff --git a/src/LiveComponent/src/Util/QueryStringPropsExtractor.php b/src/LiveComponent/src/Util/QueryStringPropsExtractor.php
@@ -43,12 +43,10 @@ public function extract(Request $request, LiveComponentMetadata $metadata, objec
         $data = [];
 
         foreach ($metadata->getAllLivePropsMetadata() as $livePropMetadata) {
-            if ($queryStringMapping = $livePropMetadata->getQueryStringMapping()) {
-                if (null !== ($value = $query[$queryStringMapping['name']] ?? null)) {
-                    if (\is_array($value) && $this->isNumericIndexedArray($value)) {
-                        // Sort numeric array
-                        ksort($value);
-                    } elseif ('' === $value && null !== $livePropMetadata->getType() && (!$livePropMetadata->isBuiltIn() || 'array' === $livePropMetadata->getType())) {
+            if ($livePropMetadata->queryStringMapping()) {
+                $frontendName = $livePropMetadata->calculateFieldName($component, $livePropMetadata->getName());
+                if (null !== ($value = $query[$frontendName] ?? null)) {
+                    if ('' === $value && null !== $livePropMetadata->getType() && (!$livePropMetadata->isBuiltIn() || 'array' === $livePropMetadata->getType())) {
                         // Cast empty string to empty array for objects and arrays
                         $value = [];
                     }
@@ -70,15 +68,14 @@ public function extract(Request $request, LiveComponentMetadata $metadata, objec
         return $data;
     }
 
-    private function isNumericIndexedArray(array $array): bool
-    {
-        return 0 === \count(array_filter(array_keys($array), 'is_string'));
-    }
-
     private function isValueTypeConsistent(mixed $value, LivePropMetadata $livePropMetadata): bool
     {
         $propType = $livePropMetadata->getType();
 
+        if ($livePropMetadata->allowsNull() && null === $value) {
+            return true;
+        }
+
         return
             \in_array($propType, [null, 'mixed'])
             || $livePropMetadata->isBuiltIn() && ('\is_'.$propType)($value)

diff --git a/src/LiveComponent/tests/Fixtures/Component/ComponentWithUrlBoundProps.php b/src/LiveComponent/tests/Fixtures/Component/ComponentWithUrlBoundProps.php
@@ -1,5 +1,14 @@
 <?php
 
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
 namespace Symfony\UX\LiveComponent\Tests\Fixtures\Component;
 
 use Symfony\UX\LiveComponent\Attribute\AsLiveComponent;
@@ -10,20 +19,22 @@
 #[AsLiveComponent('component_with_url_bound_props')]
 class ComponentWithUrlBoundProps
 {
-    #[LiveProp(writable: true, url: true)]
+    use DefaultActionTrait;
+    #[LiveProp(queryMapping: true)]
     public ?string $prop1 = null;
 
-    #[LiveProp(writable: true, url: true)]
+    #[LiveProp(queryMapping: true)]
     public ?int $prop2 = null;
 
-    #[LiveProp(writable: true, url: true)]
+    #[LiveProp(queryMapping: true)]
     public array $prop3 = [];
 
-    #[LiveProp(writable: true)]
+    #[LiveProp]
     public ?string $prop4 = null;
 
-    #[LiveProp(writable: ['address', 'city'], url: true)]
+    #[LiveProp(queryMapping: true)]
     public ?Address $prop5 = null;
 
-    use DefaultActionTrait;
+    #[LiveProp(fieldName: 'field6', queryMapping: true)]
+    public ?string $prop6 = null;
 }
diff --git a/...iveComponent/tests/Fixtures/templates/components/component_with_url_bound_props.html.twig b/...iveComponent/tests/Fixtures/templates/components/component_with_url_bound_props.html.twig
@@ -3,4 +3,6 @@
     Prop2: {{ prop2 }}
     Prop3: {{ prop3|join(',') }}
     Prop4: {{ prop4 }}
+    Prop5: address: {{ prop5.address ?? '' }} city: {{ prop5.city ?? '' }}
+    Prop6: {{ prop6 }}
 </div>
diff --git a/src/LiveComponent/tests/Functional/EventListener/AddLiveAttributesSubscriberTest.php b/src/LiveComponent/tests/Functional/EventListener/AddLiveAttributesSubscriberTest.php
@@ -149,6 +149,7 @@ public function testQueryStringMappingAttribute()
             'prop2' => ['name' => 'prop2'],
             'prop3' => ['name' => 'prop3'],
             'prop5' => ['name' => 'prop5'],
+            'field6' => ['name' => 'field6'],
         ];
 
         $this->assertEquals($expected, $queryMapping);

diff --git a/src/LiveComponent/tests/Functional/EventListener/QueryStringInitializerSubscriberTest.php b/src/LiveComponent/tests/Functional/EventListener/QueryStringInitializerSubscriberTest.php
@@ -21,12 +21,14 @@ class QueryStringInitializerSubscriberTest extends KernelTestCase
     public function testQueryStringPropsInitialization()
     {
         $this->browser()
-            ->get('/render-template/render_component_with_url_bound_props?prop1=foo&prop2=42&prop3[]=foo&prop3[]=bar&prop4=unbound')
+            ->get('/render-template/render_component_with_url_bound_props?prop1=foo&prop2=42&prop3[]=foo&prop3[]=bar&prop4=unbound&prop5[address]=foo&prop5[city]=bar&field6=foo')
             ->assertSuccessful()
             ->assertContains('Prop1: foo')
             ->assertContains('Prop2: 42')
             ->assertContains('Prop3: foo,bar')
             ->assertContains('Prop4:')
+            ->assertContains('Prop5: address: foo city: bar')
+            ->assertContains('Prop6: foo')
         ;
     }
 }
diff --git a/src/LiveComponent/tests/Functional/Metadata/LiveComponentMetadataFactoryTest.php b/src/LiveComponent/tests/Functional/Metadata/LiveComponentMetadataFactoryTest.php
@@ -30,22 +30,16 @@ public function testQueryStringMapping()
             $propsMetadataByName[$propMetadata->getName()] = $propMetadata;
         }
 
-        $this->assertEquals([
-            'name' => 'prop1',
-        ], $propsMetadataByName['prop1']->getQueryStringMapping());
+        $this->assertTrue($propsMetadataByName['prop1']->queryStringMapping());
 
-        $this->assertEquals([
-            'name' => 'prop2',
-        ], $propsMetadataByName['prop2']->getQueryStringMapping());
+        $this->assertTrue($propsMetadataByName['prop2']->queryStringMapping());
 
-        $this->assertEquals([
-            'name' => 'prop3',
-        ], $propsMetadataByName['prop3']->getQueryStringMapping());
+        $this->assertTrue($propsMetadataByName['prop3']->queryStringMapping());
 
-        $this->assertEquals([], $propsMetadataByName['prop4']->getQueryStringMapping());
+        $this->assertFalse($propsMetadataByName['prop4']->queryStringMapping());
 
-        $this->assertEquals([
-            'name' => 'prop5',
-        ], $propsMetadataByName['prop5']->getQueryStringMapping());
+        $this->assertTrue($propsMetadataByName['prop5']->queryStringMapping());
+
+        $this->assertTrue($propsMetadataByName['prop6']->queryStringMapping());
     }
 }
diff --git a/src/LiveComponent/tests/Functional/Util/QueryStringPropsExtractorTest.php b/src/LiveComponent/tests/Functional/Util/QueryStringPropsExtractorTest.php
@@ -45,19 +45,25 @@ public function testExtract(string $queryString, array $expected)
     public function getQueryStringTests(): iterable
     {
         yield from [
-            ['', []],
-            ['prop1=foo', ['prop1' => 'foo']],
-            ['prop2=42', ['prop2' => 42]],
-            ['prop3[]=foo&prop3[]=bar', ['prop3' => ['foo', 'bar']]],
-            ['prop4=foo', []], // not bound
-            ['prop5[address]=foo&prop5[city]=bar', ['prop5' => (function () {
+            'no query string' => ['', []],
+            'empty value for nullable string' => ['prop1=', ['prop1' => null]],
+            'string value' => ['prop1=foo', ['prop1' => 'foo']],
+            'empty value for nullable int' => ['prop2=', ['prop2' => null]],
+            'int value' => ['prop2=42', ['prop2' => 42]],
+            'array value' => ['prop3[]=foo&prop3[]=bar', ['prop3' => ['foo', 'bar']]],
+            'array value indexed' => ['prop3[1]=foo&prop3[0]=bar', ['prop3' => [1 => 'foo', 0 => 'bar']]],
+            'not bound prop' => ['prop4=foo', []],
+            'object value' => ['prop5[address]=foo&prop5[city]=bar', ['prop5' => (function () {
                 $address = new Address();
                 $address->address = 'foo';
                 $address->city = 'bar';
 
                 return $address;
             })(),
             ]],
+            'invalid scalar value' => ['prop1[]=foo&prop1[]=bar', []],
+            'invalid array value' => ['prop3=foo', []],
+            'invalid object value' => ['prop5=foo', []],
         ];
     }
 }
diff --git a/src/LiveComponent/tests/Unit/Metadata/LiveComponentMetadataTest.php b/src/LiveComponent/tests/Unit/Metadata/LiveComponentMetadataTest.php
@@ -22,10 +22,10 @@ class LiveComponentMetadataTest extends TestCase
     public function testGetOnlyPropsThatAcceptUpdatesFromParent()
     {
         $propMetadatas = [
-            new LivePropMetadata('noUpdateFromParent1', new LiveProp(updateFromParent: false), null, false, false, null),
-            new LivePropMetadata('noUpdateFromParent2', new LiveProp(updateFromParent: false), null, false, false, null),
-            new LivePropMetadata('yesUpdateFromParent1', new LiveProp(updateFromParent: true), null, false, false, null),
-            new LivePropMetadata('yesUpdateFromParent2', new LiveProp(updateFromParent: true), null, false, false, null),
+            new LivePropMetadata('noUpdateFromParent1', new LiveProp(updateFromParent: false), null, false, false, null, false),
+            new LivePropMetadata('noUpdateFromParent2', new LiveProp(updateFromParent: false), null, false, false, null, false),
+            new LivePropMetadata('yesUpdateFromParent1', new LiveProp(updateFromParent: true), null, false, false, null, false),
+            new LivePropMetadata('yesUpdateFromParent2', new LiveProp(updateFromParent: true), null, false, false, null, false),
         ];
         $liveComponentMetadata = new LiveComponentMetadata(new ComponentMetadata([]), $propMetadatas);
         $inputProps = [

diff --git a/src/TwigComponent/src/Event/PostMountEvent.php b/src/TwigComponent/src/Event/PostMountEvent.php
@@ -25,7 +25,7 @@ final class PostMountEvent extends Event
     public function __construct(
         private object $component,
         private array $data,
-        $metadata,
+        array|ComponentMetadata $metadata = [],
         $extraMetadata = []
     ) {
         if (\is_array($metadata)) {