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 phpstan template annotations to collection/criteria lib objects #1657

Closed

Conversation

nederdirk
Copy link
Contributor

To Do: annotations in generated classes

@dereuromark
Copy link
Contributor

Nice. this looks quite useful :)

@nederdirk
Copy link
Contributor Author

Nice. this looks quite useful :)

Thank you for your encouragement. I noticed that some of the types in our application code didn't resolve well, so I set out to solve the upstream instead of adding a lot of rules to our baseline

@dereuromark
Copy link
Contributor

Do you plan on adding more? As your current PR is still a draft.

@nederdirk
Copy link
Contributor Author

Do you plan on adding more? As your current PR is still a draft.

I do, and I plan on continuing this work on Thursday 15 October

@nederdirk
Copy link
Contributor Author

Dear @dereuromark,

while I was busy with this, I ran into this problem: ModelCriteria objects have a property $formatter (inherited from BaseModelCriteria) which determines the output type of all functions querying the database backend. One of the possible formatters is ArrayFormatter, which returns the records as an ArrayCollection<array<string,mixed>>. This poses a problem for phpstan, as it (correctly) can't unify ArrayCollection<array<string,mixed>> with Collection<T of ActiveRecordInterface>.

What would you think is the best way for me to go forward with this PR? I don't want to add type signatures that lie about the parameter or return types, but having find() return Collection<T>|Collection<array<string,mixed>> defeats the purpose of this PR; it would still result in a lot of assertions in calling code, thus not effectively increasing the value of static analysis.

@dereuromark dereuromark requested a review from a team October 15, 2020 13:32
This is done by adding TReturn and TColl template parameters,
unfortunately this is necessary: it reveals complexity that's already
present in the code.
@dereuromark
Copy link
Contributor

I would try to keep the existing param/return as is, and instead focus on adding those as phpstan-* additions on top.
This way they are optional and also helpful for our CI validation runs.

@nederdirk nederdirk marked this pull request as ready for review November 19, 2020 16:08
Copy link
Contributor Author

@nederdirk nederdirk left a comment

Choose a reason for hiding this comment

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

@dereuromark Thank you for your quick reply! I don't fully understand what you mean, and I have tried to be meticulous with not touching existing @var-annotations or syntax type hints unless absolutely necessary, or where the existing @var-annotations were incorrect. Can you perhaps point out specific instances that keep you from considering this PR for merging?

@@ -247,7 +256,7 @@ public function exchangeArray($input)
*
* @return array
*/
public function getArrayCopy()
public function getArrayCopy($keyColumn = null, $usePrefix = false)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This makes OnDemandCollection::getArrayCopy compatible with its parent's ObjectCollection::getArrayCopy

@dereuromark
Copy link
Contributor

Are you able to rebase and get ci green? Then we can continue.

@szepeviktor
Copy link

IF you write a small PHPStan extension you can reduce the number of ignores to zero.

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.

3 participants