Utilizing Expression::visit method instead of switch-case

[oleg-andreyev]

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

[rela589n]

Hi @oleg-andreyev ,

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

@Ocramius , what do you think about it?

[Ocramius]

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

[rela589n]

@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]

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

[greg0ire]

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

[greg0ire]

Link to the duplicated code:

collections/src/Expr/ExpressionVisitor.php

Lines 43 to 52 in b1f7c33

	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.