The reported WriteInfo of readonly promoted properties is incorrectly returned as a writeable property when constructor extraction is disabled. This PR fixes that by correctly returningPropertyWriteInfo::TYPE_NONE whenenable_constructor_extraction isfalse.

Obviously, this fix only applies to PHP8.1 or higher.

CasvanDongen requested a review fromdunglas as acode owner

November 4, 2022 09:51

carsonbot added Status: Needs Review Bug PropertyAccess labels

Nov 4, 2022

carsonbot added this to the5.4 milestone

Nov 4, 2022

Copy link

carsonbot commentedNov 4, 2022

Hey!

I see that this is your first PR. That is great! Welcome!

Symfony has acontribution guide which I suggest you to read.

In short:

Always add tests
Keep backward compatibility (seehttps://symfony.com/bc).
Bug fixes must be submitted against the lowest maintained branch where they apply (seehttps://symfony.com/releases)
Features and deprecations must be submitted against the 6.2 branch.

Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change.

When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

CasvanDongen changed the title~~[PropertyAccess] Promoted readonly properties must have no PropertyWriteInfo~~[PropertyInfo] Promoted readonly properties must have no PropertyWriteInfo

Nov 4, 2022

Copy link

carsonbot commentedNov 5, 2022

Hey!

I think@Tomanhez has recently worked with this code. Maybe they can help review this?

Cheers!

Carsonbot

Copy link

ContributorAuthor

CasvanDongen commentedMar 21, 2023

@dunglas or@nicolas-grekas is it possible to review this PR?

nikophil approved these changes

Apr 4, 2023

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels

Apr 4, 2023

carsonbot changed the title~~[PropertyInfo] Promoted readonly properties must have no PropertyWriteInfo~~[PropertyAccess] Promoted readonly properties must have no PropertyWriteInfo

Apr 4, 2023

Copy link

Contributor

nikophil commentedApr 4, 2023

@dunglas @nicolas-grekas any chance to see this merged? 🙏

nicolas-grekas changed the title~~[PropertyAccess] Promoted readonly properties must have no PropertyWriteInfo~~[PropertyAccess] Readonly properties must have no PropertyWriteInfo

Apr 17, 2023

nicolas-grekas reviewed

Apr 17, 2023

View reviewed changes

src/Symfony/Component/PropertyInfo/Extractor/ReflectionExtractor.php Outdated


		if ($reflClass->hasProperty($property) && ($reflClass->getProperty($property)->getModifiers() &$this->propertyReflectionFlags)) {
		$reflProperty =$reflClass->getProperty($property);
		if (\PHP_VERSION_ID <80100 \|\| (false ===$reflProperty->isReadOnly() &&false ===$reflProperty->isPromoted())) {

Copy link

Member

nicolas-grekasApr 17, 2023

Choose a reason for hiding this comment

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

If I remove the part about$reflProperty->isPromoted() (which I just did and pushed), tests still pass. This makes me wonder what how this relates to property promotion? Isn't this only about readonly?