Fix the sub-query part in UserSelection::entityQueryAlter

The connection handling is not the problem here. It is a problem on it's own, but not the reason for this problem.

The *$query* object is the one responsible for ensuring unique placeholders, but the $query that we have here already is a subquery and there is no way to get to the actual parent query within the alter hook as the alter hook is invoked for every subquery separately.

I really have no idea how to fix this other making the hack even crazier and replacing the dynamic placeholder with preg_replace() with one that we can be fairly sure to be unique.. or build a LIKE condition query on our own, but that's not easy as we need to consider database backend specific handling like PostgreSQL's ILIKE.

Berdir: Why is that necessary?

From a quick glance at the code, I don't quite get why it's even an issue. Don't use condition(). Use where(), which lets you put in whatever arbitrary SQL fragment you want and let the placeholders get compiled later as normal. Why wouldn't that work?

Because we're talking about a LIKE condition (dynamic operator actually), which needs that ESCAPE stuff, needs to use ILIKE on PostgreSQL and so on.. we'd end up rebuilding quite a bit of the code in Condition::compile() and don't have access to the information we need from there (like Condition::mapConditionOperator()).

The only doable workaround that I can think of currently would be to match and replace the dynamic placeholder string with something that we know to be unique.

I'm wondering why we made the unique placeholder stuff part of the query object and not the connection? We could for example reset it after every executed query if we're worried about generating high numbers... Would make this much easier...

Can we somehow move forward with this one? #2144377: Entity reference autocomplete lists entity labels only in current content language has been postponed forever on this :(

I *think* we made it part of the query so that each query would reset the counter, not due to an aversion to big numbers but to make the variation in the query string less. The query string has to be byte-identical in order for it to be reused by the database or driver and not recompiled.

That said, in practice I think we found that the query cache was basically useless, at least on MySQL. I don't know that it actually matters anymore. I do not know how invasive it would be to move the placeholder logic to the connection, but I'm open to trying it. It would be an API change, but one that affects only DB driver authors of whom there are about 4. :-) I feel that's safe to make up until RC, but can be overruled.

So I guess the extent of API change required here would only make this possible if done before the beta? Or we need to live with some broken core features for a while.

IMO, this is contained enough that it can happen up to RC. I defer to Dratchipott, however.

Or we could store the anonymous name in the database and remove the need for that dark magic sub-query? User entities are a translatable entity type after all and we can make the form at config/people/accounts update the name field for user 0.

But that's only based on the assumption that we don't store the anonymous user name in the db for historical reasons, before having translatable entities... I hope @Gábor can answer this better :)

Well, usernames are not supposed to be translatable even though user entities are. It would be a nightmare that if you are on /de/user, you would need to log in using your German username while on /fr/user you would need to log in with your French name. The only thing translatable among user base fields currently is the signature.

Right, then how about doing the matching for the anonymous user in php instead of the db query. Too hacky? :)

Answering to myself.. yes, too hacky because some other code might rely on the query instead of the output of SelectionInterface::getReferenceableEntities(). I'm out of simple solutions so we're back to waiting on the "Needs committer feedback" tag...

Let's get an updated summary for committers to give feedback on. :) Could use a beta evaluation: Why is it major? What is the impact of fixing this bug? Are there multiple solutions possible? What is the disruption introduced by making the change between betas (impact on contrib code, scope of change needed in core, sites running on betas, etc.)?

Re-tag I guess when ready.

I *think* we made it part of the query so that each query would reset the counter, not due to an aversion to big numbers but to make the variation in the query string less.

The code to keep this optimization while moving the counter to the Connection object is quite trivial :)

Now with less silliness.

#notmyday

+++ b/core/lib/Drupal/Core/Database/Connection.php
@@ -1409,6 +1418,27 @@ public function quote($string, $parameter_type = \PDO::PARAM_STR) {
+  public function getNextPlaceholder(PlaceholderInterface $object) {

Why do we not use the same method name as on the query object itself?

No real reason, it just sounded better.

Let's try this.

Yay unit tests!

Rerolled.

Reroll & fix comment in connection

Fixed those fails.

@amateescu I'd like to rtbc it but still needs summary update, extra tests not needed I guess

Rerolled patch in #40, thanks!

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.

This was tagged for issue summary update and tests from what I can still need to happen

Did not test yet.