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

Fix AbstractPlatform::EventManager property phpdoc #4409

Closed
wants to merge 1 commit into from

Conversation

mvorisek
Copy link
Contributor

@mvorisek mvorisek commented Nov 4, 2020

Q A
Type improvement
BC Break no
Fixed issues no

@mvorisek
Copy link
Contributor Author

mvorisek commented Nov 5, 2020

should that two Psalm warnings be fixed by allowing to set null in setEventManager method?

@greg0ire
Copy link
Member

greg0ire commented Nov 5, 2020

IMO no. I'd rather have people check with a hasEventManager method, and I think people should get an exception in 3.0.x when calling getEventManager and it returns null.

@mvorisek
Copy link
Contributor Author

mvorisek commented Nov 5, 2020

IMO no. I'd rather have people check with a hasEventManager method, and I think people should get an exception in 3.0.x when calling getEventManager and it returns null.

because too much assertions needed in static analysis?

https://github.com/doctrine/dbal/blob/master/src/Platforms/AbstractPlatform.php#L112 this is how it looks at master

@greg0ire
Copy link
Member

greg0ire commented Nov 5, 2020

because too much assertions needed in static analysis?

Yes, and also because people that don't use SA should get a better error message than "call to method foo() on null", and they should get the error message as soon as possible as opposed to when they attempt to call the method on null. That's only my opinion though, and since it is like you pointed out on master, maybe it would be great to get @morozov 's opinion too.

@morozov
Copy link
Member

morozov commented Nov 5, 2020

This is already fixed in 3.0.x (#3932) and enforced by the strict PHPStan rules. I don't think we should be making or backporting any type changes that are not enforced by static analysis (see #4346 which should have been part of #4319).

@morozov
Copy link
Member

morozov commented Nov 5, 2020

I'd rather have people check with a hasEventManager method, and I think people should get an exception in 3.0.x when calling getEventManager and it returns null.

This get/set/has approach is just a glorified global variable. Object methods should be worded in terms of "do".

@mvorisek
Copy link
Contributor Author

mvorisek commented Nov 5, 2020

Closing, because:

  • get/set/has - 3.0.x is not named that way
  • 2.12.x - is annotated not correctly, but you do not want to update

@mvorisek mvorisek closed this Nov 5, 2020
@mvorisek mvorisek deleted the fix_phpdoc branch November 5, 2020 16:10
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants