Skip to content

[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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from

Conversation

Nyholm
Copy link
Member

@Nyholm Nyholm commented Oct 31, 2020

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 method setPreviousException(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
Copy link
Member Author

Nyholm commented Oct 31, 2020

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

@jderusse
Copy link
Member

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

Copy link
Member

@jderusse 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?.

@Nyholm
Copy link
Member Author

Nyholm commented Oct 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.

@nicolas-grekas nicolas-grekas added this to the 5.2 milestone Nov 2, 2020
@fabpot
Copy link
Member

fabpot commented Nov 17, 2020

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

@fabpot fabpot modified the milestones: 5.2, 5.x Nov 30, 2020
@Nyholm Nyholm changed the base branch from 5.x to 5.2 February 15, 2021 16:07
@Nyholm Nyholm force-pushed the propertyInfo-nullable branch from 8a750ce to 756b9bc Compare February 15, 2021 16:08
Copy link
Member

@nicolas-grekas 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?

@nicolas-grekas nicolas-grekas modified the milestones: 5.4, 5.3 Sep 8, 2021
@fabpot
Copy link
Member

fabpot commented Oct 30, 2021

@Nyholm What's the status here?

@fabpot
Copy link
Member

fabpot commented Jul 25, 2022

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

@Nyholm
Copy link
Member Author

Nyholm commented Jul 25, 2022

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.

@Nyholm Nyholm closed this Jul 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants