doctrine
Utilizing Expression::visit method instead of switch-case
While working with Criteria API, I noticed that visit method in Expression is never used and duplicate logic exists in ExpressionVisitor::dispatch
By calling visit, a developer can implement his own "logic" and hook into ExpressionVisitor
My use case: I was trying to implement my own "Comparison" to work with a discriminator field but I can't add any logic to "visit" method because it's not called.
Ref.: https://github.com/doctrine/orm/issues/5908
Hi @oleg-andreyev ,
Thanks for submitting this issue. I myself has stumbled across it few times.
@Ocramius , what do you think about it?
@Ocramius , who could help us with this? Is there anyone responsible for merge requests review or any other kind of feedback (even if it is just closing the MR)? It looks like currently there are a lot of open issues and MRs back from 2020
@rela589n I'm no longer a maintainer, but @doctrine/common-code-maintainers could help
@rela589n why don't you just take a look at recently reviewed PRs, to see who reviews them? :slightly_smiling_face:
Link to the duplicated code: https://github.com/doctrine/collections/blob/b1f7c33ebb2d73a4860d6b65a11b9662b9904ebd/src/Expr/ExpressionVisitor.php#L43-L52
I noticed that visit method in Expression is never used
Can one of you please take a look at the git log and find out if that has always been the case, and if not, since when that method is no longer used? Also, can you please determine which piece of code was introduced first, and if there were any comments about the duplication in either the commit introducing the piece of code that was introduced last, or in the merge request that contains that commit? Thanks.
Hello! I strumbled across this issue recently and did some digging in hopes I can help.
Can one of you please take a look at the git log and find out if that has always been the case, and if not, since when that method is no longer used? Also, can you please determine which piece of code was introduced first, and if there were any comments about the duplication in either the commit introducing the piece of code that was introduced last, or in the merge request that contains that commit? Thanks.
According to the git log, the first version of ExpressionVisitor already had an if-else version of the current code instead of using Expression::visit(). (f09642b)
I reviewed the messages from previous and posterior commits and I could not find anything about the reasons for this choice.
Since that first version, the major changes were replacing the if-else chain with a switch statement (577975ae), and the switch with a match expression (9887248c).
Unfortunately I do not remember seeing before nor I found the duplication while looking at the code, so I have no clue on that.
@greg0ire , what do you think about this? :point_up:
I think there are no reasons for keeping that match(true) any longer and IMO using visitor's double dispatch approach would be far better
I agree. @oleg-andreyev please rebase this on 2.3.x (I think this is more of a feature than it is a bugfix, in that it probably won't make any issue in existing applications disappear). I will release 2.3.0 shortly after this gets merged.
@greg0ire please review https://github.com/doctrine/collections/pull/453 . The same changes but rebased on 2.3.x
