Support expr in queries #2343
Support expr in queries #2343
Conversation
… builders + add createExpr and depreciation notice for Builder::expr for further revision of exprOp method naming to expr
+ add contextual missing type hint for exprOr
|
@Protocteur thanks for your PR and kind words! This one will need to wait for @alcaeus' input as he's the one with the vision for aggregations and operators :) In the meantime could you please rebase your work atop the |
There was a problem hiding this comment.
Thank you @Protocteur for your contribution! I took a look through the code, and I agree with the way you've decided to solve the problem. I'd suggest using this opportunity to use clearer method names to better allow distinguishing the two different expression classes. To that end, we may want to follow up with a separate PR to rename the expression classes themselves, but that's something I want to discuss with @greg0ire first (there be dragons).
I've left some naming suggestions and some notes about deprecation notices which we provide for engineers. Feel free to apply suggestions from the code review and fix changes in a separate PR. I'm not sure if there's any value in testing the individual methods since we're covering this logic in multiple other tests - I'll let you decide how much test coverage you want to add for this.
Please let me know if you need any help, and please do follow up if I lose track of this PR.
tests/Doctrine/ODM/MongoDB/Tests/Aggregation/AggregationOperatorsProviderTrait.php
Outdated
Show resolved
Hide resolved
|
@Protocteur I changed the base of this PR to 2.3.x since that's where new features go. This unfortunately picked up a number of commits. You can rebase your branch yourself, or let us know if you require assistance with it. |
# Conflicts: # composer.json # lib/Doctrine/ODM/MongoDB/Persisters/PersistenceBuilder.php # lib/Doctrine/ODM/MongoDB/Query/Expr.php # phpstan-baseline.neon # tests/Doctrine/ODM/MongoDB/Tests/Mapping/ClassMetadataTest.php
…xprOr to aggregationExpression and createExpr to createQueryExpression and createAggregationExpression) + add depreciation triggers + refactoring associated tests - remove unnecessary exprOp in Aggregation\Expr
# Conflicts: # composer.json # lib/Doctrine/ODM/MongoDB/Persisters/PersistenceBuilder.php # lib/Doctrine/ODM/MongoDB/Query/Expr.php # phpstan-baseline.neon
|
I've updated method naming like suggested createAggregationExpression and createQueryExpression. Removed unnecesssary exprOp inside Aggregation\Expr added depreciation triggers update testing I had a lot of work on my main project that used this update, sorry for the delay ;) |
For sure you need to take a look at coding standards fail, but most probably that can be easily fixed by running |
… builders + add createExpr and depreciation notice for Builder::expr for further revision of exprOp method naming to expr
+ add contextual missing type hint for exprOr
…xprOr to aggregationExpression and createExpr to createQueryExpression and createAggregationExpression) + add depreciation triggers + refactoring associated tests - remove unnecessary exprOp in Aggregation\Expr
…ort-expr-in-queries # Conflicts: # lib/Doctrine/ODM/MongoDB/Iterator/PrimingIterator.php # lib/Doctrine/ODM/MongoDB/Mapping/ClassMetadata.php
… builders + add createExpr and depreciation notice for Builder::expr for further revision of exprOp method naming to expr
+ add contextual missing type hint for exprOr
…xprOr to aggregationExpression and createExpr to createQueryExpression and createAggregationExpression) + add depreciation triggers + refactoring associated tests - remove unnecessary exprOp in Aggregation\Expr
…pport-expr-in-queries # Conflicts: # lib/Doctrine/ODM/MongoDB/Aggregation/Builder.php # lib/Doctrine/ODM/MongoDB/Aggregation/Expr.php # lib/Doctrine/ODM/MongoDB/Aggregation/Stage/MatchStage.php # lib/Doctrine/ODM/MongoDB/Configuration.php # lib/Doctrine/ODM/MongoDB/Persisters/DocumentPersister.php # lib/Doctrine/ODM/MongoDB/Query/Builder.php
There was a problem hiding this comment.
I've matched your naming suggestions and others changed asked.
I also rebased my branch on 2.5.x because i need the lookup pipeline that has beend included in this branch.
I'm not used to review process, and didn't took time before to finish the PR/review process. But now i finished my 2 year of crunch (ouch), and i felt i had done everything right (include suggestions in commits) but the PR was still "in stand by". Now i understand (?) that i need to submit my own review to "close" the request and not only commit and comment each suggests from the request. I'm i right ?
composer.json
Outdated
| @@ -22,7 +22,7 @@ | |||
| { "name": "Andreas Braun", "email": "alcaeus@alcaeus.org" } | |||
| ], | |||
| "require": { | |||
| "php": "^7.4 || ^8.0", | |||
| "php": "^8.1", | |||
There was a problem hiding this comment.
Welcome back! 🎉
What happened here? We're definitely not going to make this bump for 2.5.x
There was a problem hiding this comment.
i roll it back, i skipped a bit of digging. And tought you were aiming 8.1 by using ReturnTypeWillChange attribute. And then i read a ref about this and saw it's design for backward compatibility xD
But i got the same error trigger localy as in the workflow about the "ReturnTypeWillChange does not exist", but after updating my php interpreter and extensions used for running phpstan/psalm localy thoses errors aren't triggered anymore. I doesn't have much knowledge around coding standard etc .. (i'm working alone for 15 years now, and unfortunatly doesn't had the occasion to pratice that part) So i'm reading a lot and try to finish this PR the correct way, but i'm really new to team work with this "level" of standard/workflow, and it's a bit rough :p, i'm currently reading psalm docs to not fucked up things. But i feel a bit out of my league on this whole subjects. Not sure i'm doing the correct way (and spam commits while i understand what i'm doing wrong or good xD)
…xpression() + replaced Aggregation\Builder::expr() calls to Aggregation\Builder::createAggregationExpression() + minor psalm docblock update
Protocteur
force-pushed
the
support-expr-in-queries
branch
from
1d55a1d to
eb60a6f
Compare
Summary
It's my first contribution.. so i'm not sure i'm doing it the right way ... but anyway here what i've done :
ps: thx for your work/time on this lib, that's really helpfull ;)