Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Set properties autowired with @required as initialized #348

Merged
merged 1 commit into from
Sep 29, 2023
Merged

Set properties autowired with @required as initialized #348

merged 1 commit into from
Sep 29, 2023

Conversation

raalderink
Copy link
Contributor

Fixes #346

@raalderink raalderink force-pushed the set-properties-marked-required-initialized branch from d9b295f to 238cd47 Compare April 21, 2023 12:44
@raalderink raalderink force-pushed the set-properties-marked-required-initialized branch 6 times, most recently from f962e83 to 45e92c3 Compare April 25, 2023 11:45
@raalderink
Copy link
Contributor Author

@ondrejmirtes could you re-review this PR please? Thank you :)

ondrejmirtes
ondrejmirtes requested changes May 23, 2023
View reviewed changes
tests/Symfony/RequiredAutowiringExtensionTest.php Outdated
$container->getServicesByTag(AdditionalConstructorsExtension::EXTENSION_TAG);

return new UninitializedPropertyRule(
new ConstructorsHelper(
Copy link
Member

Choose a reason for hiding this comment

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

Please fetch the rule with $container->getByType(UninitializedPropertyRule::class). It should be possible. That way it doesn't have to be in the baseline.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've done that and removed it from the baseline, however, it still fails on the same thing;
Accessing PHPStan\Rules\Properties\UninitializedPropertyRule::class is not covered by backward compatibility promise. The class might change in a minor PHPStan version.

src/Symfony/RequiredAutowiringExtension.php Outdated
public function getAdditionalConstructors(ClassReflection $classReflection): array
{
$additionalConstructors = [];
/** @var ReflectionClass $nativeReflection */
Copy link
Member

Choose a reason for hiding this comment

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

Please remove the inline @var.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On PHP 7.3 and lower this will throw the following;
Call to method getMethods() on an unknown class PHPStan\BetterReflection\Reflection\Adapter\ReflectionEnum.
I'm not quite sure what to do with this, as I'm confused as to why it would do this only on PHP 7.3 and lower

src/Symfony/RequiredAutowiringExtension.php Outdated Show resolved Hide resolved
tests/Type/Symfony/ExtensionTest.php Outdated Show resolved Hide resolved
@raalderink raalderink force-pushed the set-properties-marked-required-initialized branch 5 times, most recently from ef1b4bb to cee621e Compare July 31, 2023 12:51
ondrejmirtes
ondrejmirtes reviewed Aug 7, 2023
View reviewed changes
composer.json Outdated Show resolved Hide resolved
@SVillette SVillette mentioned this pull request Aug 8, 2023
Closed
@raalderink raalderink force-pushed the set-properties-marked-required-initialized branch 4 times, most recently from 7891218 to ab20c93 Compare August 16, 2023 12:26
@raalderink
Copy link
Contributor Author

raalderink commented Aug 16, 2023 •
edited

I have a couple issues here which I'm having a hard time getting resolved, as they fail on some PHP versions and succeed on others. I've baselined them, but unfortunately reportUnmatchedIgnoredErrors is on, which means it will still fail on some and succeed on others.

This following error is actually also baselined in phpstan-src, in enum-adapter-errors.neon. Due to that, I assume it's a known issue for PHP 7.2 and 7.3, but due to the reportUnmatchedIgnoredErrors, I can't really baseline this in this project. How would you want me to solve this issue?

-
	message: "#^Call to method getMethods\\(\\) on an unknown class PHPStan\\\\BetterReflection\\\\Reflection\\\\Adapter\\\\ReflectionEnum\\.$#"
	count: 1
	path: src/Symfony/RequiredAutowiringExtension.php

Secondly, on the lowest dependency builds, I get the following errors. On the highest dependencies, this builds correctly. What I've done is removed the class_exists like you asked, and instead used the class-string as string itself. I am having a lot of trouble in finding where this issue lies, and am uncertain of a solution. This is why I had the class_exists in the first place, and later tried it by including the actual symfony-contracts package that contains it (it makes sense that you don't want that to be included though). Could you please advise on how to approach this and be able to solve this issue?

-
	message: "#^Parameter \\#1 \\$name of method PHPStan\\\\BetterReflection\\\\Reflection\\\\Adapter\\\\ReflectionProperty\\:\\:getAttributes\\(\\) expects class\\-string\\|null, string given\\.$#"
	count: 1
	path: src/Symfony/RequiredAutowiringExtension.php

-
	message: "#^Parameter \\#1 \\$name of method PHPStan\\\\BetterReflection\\\\Reflection\\\\Adapter\\\\ReflectionMethod\\:\\:getAttributes\\(\\) expects class\\-string\\|null, string given\\.$#"
	count: 1
	path: src/Symfony/RequiredAutowiringExtension.php

These are the only things holding this PR back, so hopefully you can help me out by pointing me towards a solution here. Thanks @ondrejmirtes!

@ondrejmirtes ondrejmirtes force-pushed the set-properties-marked-required-initialized branch from ab20c93 to 8724f2b Compare September 29, 2023 13:56
@ondrejmirtes ondrejmirtes changed the base branch from 1.2.x to 1.3.x September 29, 2023 13:57
@ondrejmirtes ondrejmirtes force-pushed the set-properties-marked-required-initialized branch from 8724f2b to 253f8d3 Compare September 29, 2023 13:58
@ondrejmirtes ondrejmirtes force-pushed the set-properties-marked-required-initialized branch from 253f8d3 to 538586f Compare September 29, 2023 14:00
@ondrejmirtes
Copy link
Member

Solved it, mainly with phpstan/phpstan-src@2b9af36, but also locking contracts package version to a newer one :)

@ondrejmirtes ondrejmirtes force-pushed the set-properties-marked-required-initialized branch from 538586f to 80eb1d6 Compare September 29, 2023 14:09
@ondrejmirtes ondrejmirtes merged commit 3838559 into phpstan:1.3.x Sep 29, 2023
10 checks passed
@ondrejmirtes
Copy link
Member

Thank you.

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

@ondrejmirtes ondrejmirtes ondrejmirtes approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

checkUninitializedProperties does not consider @required
2 participants
@raalderink @ondrejmirtes