Skip to content

[Php80] Skip promotion when property type is wider than constructor param on ClassPropertyAssignToConstructorPromotionRector#8170

Open
ximki-vinki wants to merge 2 commits into
rectorphp:mainfrom
ximki-vinki:fix-9809-wider-property-type
Open

[Php80] Skip promotion when property type is wider than constructor param on ClassPropertyAssignToConstructorPromotionRector#8170
ximki-vinki wants to merge 2 commits into
rectorphp:mainfrom
ximki-vinki:fix-9809-wider-property-type

Conversation

@ximki-vinki

Copy link
Copy Markdown

Fixes rectorphp/rector#9809

Summary

ClassPropertyAssignToConstructorPromotionRector no longer promotes when the property type is strictly wider than the constructor parameter type (e.g. ?object property vs object param). Promotion would copy the wider property type onto the constructor and loosen the constructor contract.

Narrowing cases (e.g. object property vs ?object param) still promote as before.

Repro from the issue: https://getrector.com/demo/980360e4-321c-4a02-ac29-7062848188cf

Changes

  • Add shouldSkipWiderPropertyType() with shouldUsePropertyTypeForPromotedParam + isSubtype guard
  • Add skip_wider_property_type.php.inc — no change expected (issue repro)
  • Add narrower_property_type.php.inc — promotion with narrowing still works

Test plan

  • vendor/bin/phpunit rules-tests/Php80/Rector/Class_/ClassPropertyAssignToConstructorPromotionRector/ClassPropertyAssignToConstructorPromotionRectorTest.php
  • composer complete-check

…aram on ClassPropertyAssignToConstructorPromotionRector (#9809)
@TomasVotruba

Copy link
Copy Markdown
Member

This is not the right solution, as default null doesn't change behavior at all. The problem is in the $this->value = null.
That's what rule should look for and if found, skip it.

…opertyAssignToConstructorPromotionRector(#9809)
@ximki-vinki

Copy link
Copy Markdown
Author

This is not the right solution, as default null doesn't change behavior at all. The problem is in the $this->value = null. That's what rule should look for and if found, skip it.

Change solution


final class WiderPropertyTypeWithoutNullAssign
{
public function __construct(private(set) ?object $entity)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
public function __construct(private(set) ?object $entity)
public function __construct(private(set) object $entity)

There is no null override and constructor always set an object, so ? should be removed.


private function shouldSkipPropertyAssignedNull(Class_ $class, string $propertyName): bool
{
return (bool) $this->betterNodeFinder->findFirst(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

ClassPropertyAssignToConstructorPromotionRector promotes a property with a wider type

2 participants