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

Add mixed type hint for magic properties #443

Closed
wants to merge 1 commit into from

Conversation

dakujem
Copy link
Contributor

@dakujem dakujem commented Sep 28, 2023

  • BC break? no

Description of the problem

While using Dibi with PhpStorm (the prevalent PHP IDE), I'm constantly being warned about using void where I should not.
image

I'm pushed to either disable the inspection or suppress it for the given statement/block etc.
image

But this inspection is useful and I do not wish to suppress it.

Solution of the problem

Instead, we can just add mixed return type-hint to the magic Row::__get method to "confuse" PhpStorm.
image
We can do this, because we are actually using Row magically without type safety so there is no sacrifice to make. With mixed as the return value type, PS accepts it can not infer the type of the property and stops complaining.

We can then omit the suppressions
image

and we end up with clean code:
image

We are still warned that we acces the properties using "magic" (Property accessed via magic method).

dg pushed a commit that referenced this pull request Sep 29, 2023
@dg
Copy link
Owner

dg commented Sep 29, 2023

thanks, fixed

@dg dg closed this Sep 29, 2023
@dakujem
Copy link
Contributor Author

dakujem commented Oct 2, 2023

Hey, David, you closed this without merging to master. Not sure it was intentional. c8457ab does not close this PR, bud closes #444 instead.
@dg

@dg
Copy link
Owner

dg commented Oct 2, 2023

It is in master as part of "coding standards" commit, because all methods should have types.

@dakujem
Copy link
Contributor Author

dakujem commented Oct 3, 2023

yeah, that's better

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants