Support expr in queries #2343

Protocteur · 2021-07-18T02:52:27Z

Q	A
Type	feature
BC Break	no
Fixed issues	#1824

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 :

due to naming confusing between actual expr methods (that create new instance of expression helpers) and naming convention of expression operators that implies a future naming collision with $expr operator implementation, i've implemented new methods "createExpr" and add depreciation notice for actual expr methods and avoid BC break. Obviously with the idea that new exprOp method will then get renamed as expr in a futher release.
So i've implemented "exprOp" method throught QueryBuilder & Expr for both Query and Aggregation parts that implement usage of mongodb $expr operator
also added a test for this new implemented exprOp method/operator.

ps: thx for your work/time on this lib, that's really helpfull ;)

… 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

malarzm · 2021-07-20T20:40:11Z

@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 2.3.x branch? This will be a new feature and as such needs to go to the next minor version. Also a small heads up that if we'll go down the deprecation route there'll be couple more administrative things to do

alcaeus

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.

lib/Doctrine/ODM/MongoDB/Aggregation/Builder.php

lib/Doctrine/ODM/MongoDB/Aggregation/Expr.php

lib/Doctrine/ODM/MongoDB/Aggregation/Builder.php

lib/Doctrine/ODM/MongoDB/Aggregation/Expr.php

lib/Doctrine/ODM/MongoDB/Query/Builder.php

lib/Doctrine/ODM/MongoDB/Query/Expr.php

tests/Doctrine/ODM/MongoDB/Tests/Aggregation/AggregationOperatorsProviderTrait.php

alcaeus · 2021-07-21T00:35:03Z

@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

Protocteur · 2022-01-31T04:36:06Z

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 ;)

Protocteur

i merged my branch with doctrine:2.3.x and made requested changes.

Does it need more change now ? I saw my PR request fail at the checking process, but it seems that come from 2.3.x branch more than the change i've made. So this PR will be shelve for now ?

malarzm · 2022-02-14T13:50:07Z

Does it need more change now ? I saw my PR request fail at the checking process, but it seems that come from 2.3.x branch more than the change i've made. So this PR will be shelve for now ?

For sure you need to take a look at coding standards fail, but most probably that can be easily fixed by running vendor/bin/phpcbf locally. I'll look into fixing static analysis fails hopefully this week and will let you know once your branch can be rebased atop new 2.3.x.

… 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

Protocteur

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 ?

malarzm · 2022-11-04T22:50:17Z

composer.json

@@ -22,7 +22,7 @@
        { "name": "Andreas Braun", "email": "[email protected]" }
    ],
    "require": {
-        "php": "^7.4 || ^8.0",
+        "php": "^8.1",


Welcome back! 🎉

What happened here? We're definitely not going to make this bump for 2.5.x

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

Vincent Le Henaff added 5 commits July 17, 2021 21:41

+ implement $expr operator as exprOp method for query and aggregation…

5264e26

… builders + add createExpr and depreciation notice for Builder::expr for further revision of exprOp method naming to expr

* fix MatchStage expr revision introduced bug

4725c19

+ add exprOp test

0a38fa5

+ add contextual missing type hint for exprOr

+ fix exprOp test

c319eca

+ fix exprOp test

7ba6315

malarzm added the Feature label Jul 20, 2021

malarzm added this to the 2.x milestone Jul 20, 2021

malarzm changed the base branch from 2.2.x to 2.3.x July 20, 2021 20:37

malarzm changed the base branch from 2.3.x to 2.2.x July 20, 2021 20:37

malarzm requested a review from alcaeus July 20, 2021 20:40

alcaeus requested changes Jul 21, 2021

View reviewed changes

alcaeus changed the base branch from 2.2.x to 2.3.x July 21, 2021 00:33

alcaeus removed this from the 2.x milestone Aug 5, 2021

Vincent Le Henaff added 3 commits January 31, 2022 04:06

+ update methods name refactoring as suggested in PR doctrine#2343 (e…

06a95f1

…xprOr to aggregationExpression and createExpr to createQueryExpression and createAggregationExpression) + add depreciation triggers + refactoring associated tests - remove unnecessary exprOp in Aggregation\Expr

Merge branch '2.3.x' into support-expr-in-queries

3de0efb

# Conflicts: # composer.json # lib/Doctrine/ODM/MongoDB/Persisters/PersistenceBuilder.php # lib/Doctrine/ODM/MongoDB/Query/Expr.php # phpstan-baseline.neon

Protocteur requested a review from alcaeus January 31, 2022 04:36

Protocteur commented Feb 8, 2022

View reviewed changes

malarzm self-requested a review February 14, 2022 13:50

Vincent Le Henaff added 5 commits March 8, 2022 14:29

+ used phpcbf on /lib

fe79bbd

+ used phpcbf on /tests

898ce49

+ implement $expr operator as exprOp method for query and aggregation…

5b6a368

… builders + add createExpr and depreciation notice for Builder::expr for further revision of exprOp method naming to expr

* fix MatchStage expr revision introduced bug

e865bd0

+ add exprOp test

4285401

+ add contextual missing type hint for exprOr

Vincent Le Henaff added 6 commits March 8, 2022 15:21

+ fix exprOp test

eece492

+ fix exprOp test

8e4ea1b

+ update methods name refactoring as suggested in PR doctrine#2343 (e…

ccaa1a4

…xprOr to aggregationExpression and createExpr to createQueryExpression and createAggregationExpression) + add depreciation triggers + refactoring associated tests - remove unnecessary exprOp in Aggregation\Expr

+ rebase on newer 2.3.x

110375e

Merge remote-tracking branch 'fork/support-expr-in-queries' into supp…

caf12dd

…ort-expr-in-queries # Conflicts: # lib/Doctrine/ODM/MongoDB/Iterator/PrimingIterator.php # lib/Doctrine/ODM/MongoDB/Mapping/ClassMetadata.php

- remove test on previously removed method (aggregationExpression)

d1660cd

malarzm changed the base branch from 2.3.x to 2.4.x March 27, 2022 11:09

Vincent Le Henaff and others added 11 commits November 4, 2022 11:09

+ implement $expr operator as exprOp method for query and aggregation…

0b458de

… builders + add createExpr and depreciation notice for Builder::expr for further revision of exprOp method naming to expr

* fix MatchStage expr revision introduced bug

839adc2

+ add exprOp test

fb14785

+ add contextual missing type hint for exprOr

+ fix exprOp test

906d5bc

+ fix exprOp test

67e6913

+ update methods name refactoring as suggested in PR doctrine#2343 (e…

b387bff

…xprOr to aggregationExpression and createExpr to createQueryExpression and createAggregationExpression) + add depreciation triggers + refactoring associated tests - remove unnecessary exprOp in Aggregation\Expr

+ rebase on newer 2.3.x

372a27b

+ used phpcbf on /tests

9d5845c

- remove test on previously removed method (aggregationExpression)

c30899b

+ minor documentation updates

a3dfb0d

Protocteur changed the base branch from 2.4.x to 2.5.x November 4, 2022 10:39

+ applied vendor/bin/phpcbf

9746da5

Protocteur commented Nov 4, 2022

View reviewed changes

+ minors fixes for vendor/bin/phpstan validation

a28c84c

malarzm reviewed Nov 4, 2022

View reviewed changes

Protocteur added 2 commits November 5, 2022 02:29

+ rollback some unexpected changes in previous commits

771b43d

+ replaced Query\Builder::expr() calls to Query\Builder::createQueryE…

eb60a6f

…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 November 7, 2022 04:34

+ adding regexMatch to Aggretation

b5b2a82

alcaeus changed the base branch from 2.5.x to 2.10.x September 20, 2024 12:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support expr in queries #2343

Support expr in queries #2343

Protocteur commented Jul 18, 2021

malarzm commented Jul 20, 2021

alcaeus left a comment

alcaeus commented Jul 21, 2021

Protocteur commented Jan 31, 2022

Protocteur left a comment •

edited

Loading

malarzm commented Feb 14, 2022

Protocteur left a comment

malarzm Nov 4, 2022

Protocteur Nov 5, 2022

Support expr in queries #2343

Are you sure you want to change the base?

Support expr in queries #2343

Conversation

Protocteur commented Jul 18, 2021

Summary

malarzm commented Jul 20, 2021

alcaeus left a comment

Choose a reason for hiding this comment

alcaeus commented Jul 21, 2021

Protocteur commented Jan 31, 2022

Protocteur left a comment • edited Loading

Choose a reason for hiding this comment

malarzm commented Feb 14, 2022

Protocteur left a comment

Choose a reason for hiding this comment

malarzm Nov 4, 2022

Choose a reason for hiding this comment

Protocteur Nov 5, 2022

Choose a reason for hiding this comment

Protocteur left a comment •

edited

Loading