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

Utilizing Expression::visit method instead of switch-case #230

Open
wants to merge 2 commits into
base: 2.0.x
Choose a base branch
from

Conversation

oleg-andreyev
Copy link

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.: doctrine/orm#5908

Base automatically changed from master to 2.0.x January 19, 2021 07:24
@rela589n
Copy link

Hi @oleg-andreyev ,

Thanks for submitting this issue. I myself has stumbled across it few times.

@Ocramius , what do you think about it?

@Ocramius
Copy link
Member

@rela589n I have no memory of this place 😵‍💫

@rela589n
Copy link

@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

@Ocramius
Copy link
Member

@rela589n I'm no longer a maintainer, but @doctrine/common-code-maintainers could help

@greg0ire
Copy link
Member

@rela589n why don't you just take a look at recently reviewed PRs, to see who reviews them? 🙂

@greg0ire
Copy link
Member

Link to the duplicated code:

public function dispatch(Expression $expr)
{
return match (true) {
$expr instanceof Comparison => $this->walkComparison($expr),
$expr instanceof Value => $this->walkValue($expr),
$expr instanceof CompositeExpression => $this->walkCompositeExpression($expr),
default => throw new RuntimeException('Unknown Expression ' . $expr::class),
};
}
}

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants