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.

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

Adapt ignore error message #104

Merged
merged 2 commits into from
Dec 9, 2023
Merged

Conversation

greg0ire
Copy link
Member

@greg0ire greg0ire commented Dec 8, 2023

Looks like the "of object" part is no longer mentioned in recent PHPStan versions.

Looks like the "of object" part is no longer mentioned in recent PHPStan
versions.
@greg0ire greg0ire changed the base branch from 2.0.x to 1.5.x December 8, 2023 21:40
@greg0ire greg0ire requested review from derrabus and removed request for derrabus December 8, 2023 21:40
@greg0ire greg0ire marked this pull request as draft December 8, 2023 21:43
@greg0ire
Copy link
Member Author

greg0ire commented Dec 8, 2023

Now there is a broken Psalm build 🤦‍♂️
And I cannot reproduce it on the playground, which seems to be using the master branch for some reason. Trying to get help on this at https://symfony-devs.slack.com/archives/C8SFXTD2M/p1702073197631449

Using &$error inside the use clause of a Closure to make a variable
defined inside the closure available outside should be fine. Psalm seems
to be OK with it on the playground, which uses the master branch, which
must contain a fix, but I did not find which commit exactly on master
fixes it.
@greg0ire greg0ire marked this pull request as ready for review December 8, 2023 22:22
<file name="src/Doctrine/Instantiator/Instantiator.php" />
</errorLevel>
</UndefinedVariable>
</issueHandlers>
Copy link
Member Author

Choose a reason for hiding this comment

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

The alternative here is to add $error = null; before this line:

set_error_handler(static function (int $code, string $message, string $file, int $line) use ($reflectionClass, &$error): bool {
, but although it is surprising to have this undeclared variable being used here (at least to me), apparently, it works just fine.

Copy link
Member

Choose a reason for hiding this comment

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

I wanted to test that and clone the repository, but it seems you already did that a minute ago 😄

@greg0ire greg0ire mentioned this pull request Dec 8, 2023
Copy link
Member

@SenseException SenseException left a comment

Choose a reason for hiding this comment

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

I personally would have preferred $error = null; as it doesn't need the addition to the psalm config.

@greg0ire
Copy link
Member Author

greg0ire commented Dec 8, 2023

Yeah but that config is meant to go away in the (far?) future, and we probably would not think of removing that unneeded null initialization. Plus, if we can avoid touching working code for wrong reasons, it's probably best, right?

@SenseException
Copy link
Member

SenseException commented Dec 8, 2023

if we can avoid touching working code for wrong reasons, it's probably best, right?

I agree with this and that's why I ✅ it.

@greg0ire greg0ire merged commit 15e9c2e into doctrine:1.5.x Dec 9, 2023
15 checks passed
@greg0ire greg0ire deleted the adapt-ignore-message branch December 9, 2023 09:08
@greg0ire greg0ire added this to the 1.5.1 milestone Dec 9, 2023
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.

2 participants