Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[PropertyAccess][Serializer] Fix "type unknown" on denormalize#52879

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.

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

Already on GitHub?Sign in to your account

Merged

Conversation

@seferov
Copy link
Contributor

@seferovseferov commentedDec 3, 2023•
edited
Loading

QA
Branch?7.1
Bug fix?no
New feature?yes
Deprecations?no
LicenseMIT

The bug can clearly be seen on tests forRequestPayloadValueResolver onhttps://github.com/symfony/symfony/blob/6.3/src/Symfony/Component/HttpKernel/Tests/Controller/ArgumentResolver/RequestPayloadValueResolverTest.php#L255

// ...publicfunctiontestValidationNotPassed()  {$content ='{"price": 50, "title": ["not a string"]}';$payload =newRequestPayload(50);$serializer =newSerializer([newObjectNormalizer()], ['json' =>newJsonEncoder()]);// removed for simplificationtry {$resolver->onKernelControllerArguments($event);$this->fail(sprintf('Expected "%s" to be thrown.', HttpException::class));      }catch (HttpException$e) {$validationFailedException =$e->getPrevious();$this->assertInstanceOf(ValidationFailedException::class,$validationFailedException);$this->assertSame('This value should be of type unknown.',$validationFailedException->getViolations()[0]->getMessage());$this->assertSame('Test',$validationFailedException->getViolations()[1]->getMessage());      }  }}class RequestPayload{    #[Assert\Length(min:10, groups: ['strict'])]publicstring$title;publicfunction__construct(publicreadonlyfloat$price)    {    }}

Even thoughtitle is string, the error message shown to user is ambiguous: "This value should be of type unknown.". Instead, the message should be "This value should be of type string."

yceruto reacted with thumbs up emoji
@carsonbotcarsonbot added this to the6.3 milestoneDec 3, 2023
@carsonbotcarsonbot changed the title[Serializer] [PropertyAccess] Fix "type unknown" on denormalize[PropertyAccess][Serializer] Fix "type unknown" on denormalizeDec 3, 2023
@seferovseferovforce-pushed theserializer-fix-normalize-unknown-type branch 3 times, most recently from003dbee to927f1bdCompareDecember 4, 2023 10:58
@yceruto
Copy link
Member

Even if it looks like a bug, I have doubts about whether this should go to7.1 as an improvement.

@seferovseferovforce-pushed theserializer-fix-normalize-unknown-type branch from927f1bd to810f4ddCompareDecember 4, 2023 12:16
Copy link
Member

@nicolas-grekasnicolas-grekas left a comment

Choose a reason for hiding this comment

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

Even if it looks like a bug, I have doubts about whether this should go to 7.1 as an improvement.

I agree with this sentiment. Technically it's a new feature because it adds a new class so let's target 7.1.

@seferovseferovforce-pushed theserializer-fix-normalize-unknown-type branch from810f4dd to5eda809CompareDecember 4, 2023 17:37
@seferovseferov changed the base branch from6.3 to7.1December 4, 2023 17:37
Copy link
Member

@ycerutoyceruto left a comment

Choose a reason for hiding this comment

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

LGTM, Thank you Farhad!

seferov reacted with thumbs up emoji
Copy link
Member

@xabbuhxabbuh left a comment

Choose a reason for hiding this comment

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

The low deps failures are related. We need to bump the dev requirement onsymfony/property-access to^7.1.

@seferovseferovforce-pushed theserializer-fix-normalize-unknown-type branch from8eb8e6c tof345c20CompareDecember 6, 2023 08:39
@xabbuh
Copy link
Member

thecomposer.json file to update is located insrc/Symfony/Component/HttpKernel

seferov reacted with thumbs up emoji

@seferov
Copy link
ContributorAuthor

thecomposer.json file to update is located insrc/Symfony/Component/HttpKernel

@xabbuh correct me if I am wrong but these unit tests on HttpKernel component runs with Serializer & ProperyAccess with version 7.1.x-dev, not with the code that are on the PR. That's why they fail.

@xabbuh
Copy link
Member

@seferov Can you please rebase your changes on the latest7.1 branch?

@seferovseferovforce-pushed theserializer-fix-normalize-unknown-type branch from61acfb1 to849db42CompareDecember 7, 2023 09:35
@seferov
Copy link
ContributorAuthor

seferov commentedDec 7, 2023•
edited
Loading

@seferov Can you please rebase your changes on the latest7.1 branch?

@xabbuh done

@xabbuh
Copy link
Member

@xabbuh correct me if I am wrong but these unit tests on HttpKernel component runs with Serializer & ProperyAccess with version 7.1.x-dev, not with the code that are on the PR.

In fact, our pipeline is a bit more complex and takes the changes from the PR into account:https://github.com/symfony/symfony/blob/7.1/.github/workflows/unit-tests.yml#L91

@seferov
Copy link
ContributorAuthor

In fact, our pipeline is a bit more complex and takes the changes from the PR into account:https://github.com/symfony/symfony/blob/7.1/.github/workflows/unit-tests.yml#L91

@xabbuh I see.

Btw, only 8.2 (high deps) is failing now. The rest is passing

xabbuh reacted with thumbs up emoji

"symfony/messenger":"^6.4|^7.0",
"symfony/mime":"^6.4|^7.0",
"symfony/property-access":"^6.4|^7.0",
"symfony/property-access":"^7.1",
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need the bump here. Let's rather skip the added test when the new exception class does not exist. Otherwise we won't be able to test the Serializer component with the real lowest allowed release of the PropertyAccess component.

mtarld reacted with thumbs up emoji
@fabpotfabpot modified the milestones:6.3,7.1Dec 8, 2023
@fabpotfabpotforce-pushed theserializer-fix-normalize-unknown-type branch from34d6030 to4a665acCompareDecember 8, 2023 14:01
@fabpot
Copy link
Member

Thank you@seferov.

seferov reacted with thumbs up emoji

@yareckon
Copy link

Question,@fabpot set the milestone here for both 6.3 and 7.1, but the merge only happened for 7.1. This inscrutability for the error messages still persists on 6.4.x -- the LTS branch. Could we get an official patch for that?

@OskarStark
Copy link
Contributor

Even if it looks like a bug, I have doubts about whether this should go to 7.1 as an improvement.

I agree with this sentiment. Technically it's a new feature because it adds a new class so let's target 7.1.

This is a new feature and will not be backported to 6.4 and the milestone is set for 7.1 only.

@yareckon
Copy link

Thanks for the response even if it's not the one I hoped for.

@fabpotfabpot mentioned this pull requestMay 2, 2024
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@xabbuhxabbuhxabbuh requested changes

@fabpotfabpotfabpot approved these changes

@ycerutoycerutoyceruto approved these changes

@mtarldmtarldmtarld approved these changes

@dunglasdunglasAwaiting requested review from dunglasdunglas is a code owner

@lyrixxlyrixxAwaiting requested review from lyrixx

@chalasrchalasrAwaiting requested review from chalasr

@OskarStarkOskarStarkAwaiting requested review from OskarStark

@jderussejderusseAwaiting requested review from jderusse

@nicolas-grekasnicolas-grekasAwaiting requested review from nicolas-grekas

Assignees

No one assigned

Projects

None yet

Milestone

7.1

Development

Successfully merging this pull request may close these issues.

9 participants

@seferov@yceruto@xabbuh@fabpot@yareckon@OskarStark@nicolas-grekas@mtarld@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp