Skip to content

[LiveComponent] Fix new URL calculation with LiveProp using Serializer (and Serializer attributes), and when using custom modifier #2988

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Aug 15, 2025
Merged

[LiveComponent] Fix new URL calculation with LiveProp using Serializer (and Serializer attributes), and when using custom modifier #2988

merged 2 commits into from
Aug 15, 2025
+236 −230

Conversation

Kocal
Copy link
Member

@Kocal Kocal commented Aug 12, 2025 •
edited
Loading

Q A
Bug fix? yes
New feature? no
Docs? no
Issues Fix #2971, #2991
License MIT

This PR fixes two bugs introduced #2673:

  1. See [LiveComponent] ignored #[SerializedName(serializedName: 'p')] after 2.28 upgrade #2971 (comment) for explanations
    tl;dr: previously, query parameters were updated through LiveComponentHydrator::dehydrate() which use the Serializer (if useSerializerForHydration: true) and so also used Serializer attributes like SerializedName.
  2. [LiveComponent] LiveProp modifier for changing URL mapping no longer works after updating to 2.29.1 #2991, when having a LiveProp(url: new UrlMapping(as: 'foo')) with a custom modifier that returns a new LiveProp with a new UrlMapping, ex:
    public function myModifier(LiveProp $liveProp): LiveProp
    {
        $urlMapping = $liveProp->url();
        if (!$urlMapping instanceof UrlMapping) {
            return $liveProp;
        }

        return $liveProp->withUrl(new UrlMapping(as: 'alias_' . $urlMapping->as));
    }

then the prop was persisted as foo and not alias_foo in the URL.

@carsonbot carsonbot added Bug Bug Fix LiveComponent Status: Needs Review Needs to be reviewed labels Aug 12, 2025
Kocal
Kocal commented Aug 12, 2025
View reviewed changes
src/LiveComponent/tests/Unit/EventListener/LiveUrlSubscriberTest.php Outdated
Copy link
Member Author

@Kocal Kocal Aug 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this test was adding value, legit everything was mocked and it could not reflect the real behavior.

Instead I went for functional test

Kocal
Kocal commented Aug 12, 2025
View reviewed changes
src/LiveComponent/src/EventListener/LiveUrlSubscriber.php
Comment on lines +78 to +86
$metadata = $this->metadataFactory->getMetadata($mounted->getName());

$dehydratedProps = $this->liveComponentHydrator->dehydrate(
$mounted->getComponent(),
$mounted->getAttributes(),
$metadata
);

$values = $dehydratedProps->getProps();
Copy link
Member Author

@Kocal Kocal Aug 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the fix for the first bug

@Kocal Kocal force-pushed the live-component-2971 branch from f6361c1 to 41e73d0 Compare August 12, 2025 11:06
Kocal
Kocal commented Aug 12, 2025
View reviewed changes
src/LiveComponent/tests/Integration/LiveComponentHydratorTest.php
@@ -1064,12 +1064,43 @@ public function mount()
});
}];

yield 'Array with DTOs: using serializer, fully writable allows partial changes' => [function () {
Copy link
Member Author

@Kocal Kocal Aug 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since I added #[SerializedName(serializedName: 'c')] on Address::$city, our tests failed because we expected props to be dehydrated to city instead of c.

I duplicated these failing tests and changed the dehydrated property to c, and modified the previous tests to not use useSerializerForHydration.

@Kocal Kocal requested review from kbond and smnandre August 12, 2025 11:09
@Kocal
Copy link
Member Author

Kocal commented Aug 12, 2025

cc @mbuliard, can you take a look? Thanks!

@Kocal Kocal mentioned this pull request Aug 12, 2025
Closed
Kocal
Kocal commented Aug 12, 2025
View reviewed changes
src/LiveComponent/tests/Functional/EventListener/LiveUrlSubscriberTest.php
$address->city = 'Anytown';
yield 'with an object in query, with "useSerializerForHydration: true", keys "address" and "c" must be present' => [
'previousLocation' => '/',
'expectedLocation' => '/?intProp=3&objectPropWithSerializerForHydration%5Baddress%5D=123+Main+St&objectPropWithSerializerForHydration%5Bc%5D=Anytown',
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's not easy to read, but the c is present at 5Bc%5D

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw it would be nice if we could have a "clean" query, like it was done before.

I tried to play with http_build_query()'s encoding, but it didn't help

@Kocal Kocal mentioned this pull request Aug 12, 2025
Closed
@Kocal
Copy link
Member Author

Kocal commented Aug 12, 2025

I pushed a second commit to fix #2991, now modifier are nicely used to generate the URL

@Kocal Kocal changed the title [LiveComponent] Fix new URL calculation when having #[LiveProp] with useSerializerForHydration: true and #[SerializedName] [LiveComponent] Fix new URL logic when using #[LiveProp] and Serializer together, but also with custom modifier Aug 12, 2025
@Kocal Kocal changed the title [LiveComponent] Fix new URL logic when using #[LiveProp] and Serializer together, but also with custom modifier [LiveComponent] Fix new URL calculation with LiveProp using Serializer (and Serializer attributes), and when using custom modifier Aug 12, 2025
Kocal
Kocal commented Aug 12, 2025
View reviewed changes
src/LiveComponent/src/Metadata/LiveComponentMetadata.php
{
$urlMappings = [];
foreach ($this->livePropsMetadata as $livePropMetadata) {
foreach ($this->getAllLivePropsMetadata($component) as $livePropMetadata) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the fix for the 2nd bug

@Kocal Kocal force-pushed the live-component-2971 branch 3 times, most recently from 2be1f38 to a1b199e Compare August 14, 2025 21:45
Kocal added 2 commits August 15, 2025 07:52
@Kocal
[LiveComponent] Fix new URL calculation when having #[LiveProp] wit…
07e73bf 
…h `useSerializerForHydration: true` and `#[SerializedName]`
@Kocal
[LiveComponent] Fix new URL calculation when having #[LiveProp] wit…
19fe704 
…h custom `modifier` which returns a new `LiveProp`
@Kocal Kocal force-pushed the live-component-2971 branch from a1b199e to 19fe704 Compare August 15, 2025 05:52
@Kocal Kocal merged commit 5eb1153 into symfony:2.x Aug 15, 2025
27 checks passed
@Kocal Kocal deleted the live-component-2971 branch August 15, 2025 06:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kbond kbond Awaiting requested review from kbond

@smnandre smnandre Awaiting requested review from smnandre

Assignees
No one assigned
Labels
Bug Bug Fix LiveComponent Status: Needs Review Needs to be reviewed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[LiveComponent] ignored #[SerializedName(serializedName: 'p')] after 2.28 upgrade
2 participants
@Kocal @carsonbot