NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[PropertyInfo] Dont try to set null when argument is not nullable#38920

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

Jump to bottom

Closed

Nyholm wants to merge3 commits intosymfony:5.2fromNyholm:propertyInfo-nullable

Closed

[PropertyInfo] Dont try to set null when argument is not nullable#38920

Nyholm wants to merge3 commits intosymfony:5.2fromNyholm:propertyInfo-nullable

Conversation

Copy link

Member

Nyholm commentedOct 31, 2020•
edited by nicolas-grekas
Loading

Q	A
Branch?	5.3
Bug fix?	yes
New feature?	no
Deprecations?	no
Tickets	Part of#38800
License	MIT
Doc PR

This will break up#38800 into two smaller pieces.

If there is a methodsetPreviousException(Throwable $t) and we try to unserialise a value of ['previousException'=>null], we should not try to invoke this setter. It will cause an error.

Nyholm requested a review fromdunglas as acode owner

October 31, 2020 10:41

carsonbot added Status: Needs Review PropertyInfo Bug labels

Oct 31, 2020

Nyholm requested review fromjderusse andxabbuh

October 31, 2020 10:44

Copy link

MemberAuthor

Nyholm commentedOct 31, 2020

The testtestThrowTypeErrorWithNullArgument is failing for this. Why would the desired behavior be that we get an Error likeExpected argument of type "Throwable", "null" given?

Copy link

Member

jderusse commentedOct 31, 2020

If it's a bug fix, shouldn't it target 4.4?

jderusse reviewed

Oct 31, 2020

View reviewed changes

Copy link

Member

jderusse left a comment

Choose a reason for hiding this comment

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

What's about constructor, non nullable properties (typehinted in php 7.4) and mutators?.

src/Symfony/Component/PropertyInfo/Extractor/ReflectionExtractor.phpShow resolvedHide resolved

Copy link

MemberAuthor

Nyholm commentedOct 31, 2020

If it's a bug fix, shouldn't it target 4.4?

Last time I checked, the code was very different in 4.4 and 5.1. I can check again later.

xabbuh reviewed

Nov 2, 2020

View reviewed changes

src/Symfony/Component/PropertyAccess/PropertyAccessor.phpShow resolvedHide resolved

nicolas-grekas added this to the5.2 milestone

Nov 2, 2020

Copy link

Member

fabpot commentedNov 17, 2020

@Nyholm Can you finish this one? I think it should go in 5.2.

fabpot modified the milestones:5.2,5.x

Nov 30, 2020

Nyholm changed the base branch from5.x to5.2

February 15, 2021 16:07

PropertyInfo dont try to set null when argument is not nullable

756b9bc

Nyholm force-pushed thepropertyInfo-nullable branch from8a750ce to756b9bcCompare

February 15, 2021 16:08

Nyholm added2 commits

February 15, 2021 17:38

Added tests

29020ba

cs

6677aa7

Nyholm commented

Feb 15, 2021

View reviewed changes

src/Symfony/Component/PropertyAccess/PropertyAccessor.phpShow resolvedHide resolved

nicolas-grekas reviewed

Sep 8, 2021

View reviewed changes

Copy link

Member

nicolas-grekas left a comment

Choose a reason for hiding this comment

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

Can you please rebase to target 5.3?

src/Symfony/Component/PropertyAccess/PropertyAccessor.phpShow resolvedHide resolved

nicolas-grekas modified the milestones:5.4,5.3

Sep 8, 2021

Copy link

Member

fabpot commentedOct 30, 2021

@Nyholm What's the status here?

Copy link

Member

fabpot commentedJul 25, 2022

@Nyholm Sorry to ping you again on this one :)

Copy link

MemberAuthor

I thought i would patch a simple bug. But Im not sure this is needed. I dont even remember what I did to find this bug. Im closing this since it has not been a priority for me (or anyone else) to fix it.