Skip to content

Conversation

@BenMorel
Copy link
Contributor

Q A
Type improvement
BC Break yes
Fixed issues none

Summary

As suggested by @morozov in #3824 (comment):

To get rid of the remaining PHPStan issue, we may replace the associative array representing a join in the Query Builder with an object similar to DependencyOrderNode used in #3762. This way, we will not only help static analyzers understand which properties and of what types a join has but also may help PHP manage memory more efficiently.

This PR introduces 3 classes, which replace the $sqlParts nested associative array:

  • QueryParts
  • QueryPartFrom
  • QueryPartJoin

This is a first draft with both phpunit & phpstan happy.
A few pain points are to be discussed, though.

@morozov
Copy link
Member

morozov commented Jan 18, 2020

Originally, I wanted to start only with the Join object since this is where we had problems with the type system. Why don't we solve one problem at a time? Otherwise, it may be much slower since I already have a lot of questions.

By implementing one aspect, we'll settle on most of the questions and then implement the rest. Otherwise, we may end up discussing and changing the same things three times for each of the newly introduced classes.

@BenMorel
Copy link
Contributor Author

@morozov I don't feel like this makes it much easier. They all take their roots in $sqlParts, and apart from the few comments I've made, it's not that big of a change?

@BenMorel
Copy link
Contributor Author

But you may be right. I'll see if I can make a very small PR with just QueryPartJoin.

@morozov
Copy link
Member

morozov commented Jan 18, 2020

I'm not concerned about the scale of the change. You and I have a history of discovering huge amounts of work by starting with mid-scale changes. We can go full scale if you prefer, but I want to make sure you're not disappointed by the number of back and forths and the time it may take to get done.

@BenMorel
Copy link
Contributor Author

As long as we're getting there, I don't really mind. But you may have a point, we could possibly get there in less time by going with small increments. Let me try a very small PR for the join only, and we'll see much more clearly which path we want to follow.

@BenMorel
Copy link
Contributor Author

@morozov #3830 ready for your review! It's a bite-size chunk :)

@BenMorel
Copy link
Contributor Author

Superseded by #3836.

@BenMorel BenMorel closed this Jan 21, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 5, 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.

2 participants