User session change causes runtime exception in other modules

Hm, I'm not sure whether that's a good idea, and what the ramifications would be. While surely not perfect, the current code has worked pretty well so far, apparently, as I think you're the first one complaining about it. (Or, at least, one of the first.) If we change that, there's no telling what kind of problems people with other setups would run into. The problem, as far as I can see, is that there is no “officially supported” way of doing this, so the best we can do is hacking and hoping.

Have you tried your solution? Did it resolve the issue and not cause any other problems for you?
Then I guess maybe just keep using that patched version, but I don't think I want to risk committing such a change (unless there's a lot more complaints about the current code).

Maybe with the new memory cache more code would move to something that can be reliably “set back” to a previous state, avoiding bleeding through of such temporary session changes. (We're running into a similar problem in #2898334: Add a "Role-based access" processor.)

OK, thanks for reporting back!
I'm leaving this open in case more people want to complain, but it really seems to be a very setup-specific problem.

I've stumbled upon the same issue. In my case it was a custom hook_node_access() where I use $account->isAnonymous() and everything works perfect except the search. The thing is implementation of isAnonymous() relying on uid only
Drupal\Core\Session\UserSession:124

  public function isAnonymous() {
    return $this->uid == 0;
  }

I understand this is a tricky issue and, @drunken monkey, I share your concerns. Though seems like more attention needed here.

Posting the patch with changes @marco-s suggested.

Hm, I see, that is a bit of a problem.
However, if the admin does want to render with the “Anonymous” role, then that’s the desired behavior.

I see two possible small improvements here, that might fix at least some edge cases:

• If only the anonymous role is enabled, use AnonymousUserSession instead. (This could also be used to ensure that you can’t combine it with other roles, which would also make sense. However, that should probably come with some UI validation, too, to avoid confusion.)
• If it’s not an anonymous user, also set uid to some arbitrary value (except 1, of course).

Does that make sense to you?

Something like this?

The only thing I see that might be a problem is that there could be code doing a User::load() based on the ID in the current user service. If you use that actual loaded user you might get different roles. I thought about using a non-existent user ID, but if you do this the code using User::load() would also fail.

Not sure if there is a perfect solution to this?

Not sure if there is a perfect solution to this?

Definitely not, no. There’s just no really built-in way to do this stuff, so we could run into some problems with any solution.
But how about using a negative integer? That’s at least sure to not map to an existing user … ? This might, of course, run into some different trouble somewhere, however.

This also needed a re-roll since I just committed #2987245: The account_switcher service should be used instead of currentUser->setAccount() in RenderedItem.
Finally, I also went ahead and used AnonymousUserSession if the admin selected anonymous at all – using that in conjunction with any other roles would be a stupid idea anyways, so it’s probably safer to just not allow that. (As said, though, we might also consider adding some form validation in the UI for this.)

Revised patch attached, would be great if you could give it a try.
An automated test for this would also be great, but I’m not sure that’s easily possible. And I don’t think it’s worth too much work, either.

Unfortunately we already ran in to an issue on 1 of the environments we tested this on. Some code was calling User::load() for the current user and was not properly checking if the user actually existed (and normally it shouldn't have to do that). Then the indexing just stops at that point without a way of fixing it.

A possible alternative solution would be to configure an actual user in search API instead of a list of roles. So you could select:

• Anonymous user
• Current user
• Custom user

Anonymous user should probably be the default? The field should also have a short description to explain that the 'Current user' options could lead to different indexing results depending on who is saving the content. When you select 'Custom user' you should be able to select a specific (existing) user. This would allow editors to create a special user just for search API indexing if they really want to. This might even be something to recommend when not indexing as an anonymous user.

Besides selecting a user we should probably also encourage users to create a special view mode for indexing since fields of some modules (like for instance the flag module) are not something you should normally want to index anyway.

Is the problem here not very closely related to #2898334: Add a "Role-based access" processor? I mean, that's where we also run into similar problems with the users - we could/should create some kind of helper to make the user-switching the same.

Unfortunately we already ran in to an issue on 1 of the environments we tested this on. Some code was calling User::load() for the current user and was not properly checking if the user actually existed (and normally it shouldn't have to do that). Then the indexing just stops at that point without a way of fixing it.

Ah, damn it …
Then yes, I guess using an existing user is really the only option we have. Whether it better be one created by the Search API automatically, or one created by admins, is the next question – I see advantages for both. (The latter has the potential problem of using a normal user and then forgetting this when changing that user’s roles. But having the Search API maintain a user account might not be acceptable, or at least desirable, for some sites.)
Either way, while we now know it doesn’t really work reliably, we’ll also have to keep the old setting, I guess, for BC reasons. (No real way to automatically update this to use a user account instead – especially if we don’t want to create it ourselves.)

And, in any case, just using the anonymous user should still work the same – that’s the one thing that won’t make any problems, as far as I can see. (And is probably the most common setting, too, fortunately.)

Oh, and I don’t think “current user” makes sense. That’s just too unpredictable to be useful. (And will mostly mean “admin user”, which is also rarely desirable.)

Besides selecting a user we should probably also encourage users to create a special view mode for indexing since fields of some modules (like for instance the flag module) are not something you should normally want to index anyway.

Quoting the config form description: “We recommend using a dedicated view mode (for example, the "Search index" view mode available by default for content) to make sure that only relevant data (especially no field labels) will be included in the index.”
We can hardly mention it more prominently.

Is the problem here not very closely related to #2898334: Add a "Role-based access" processor? I mean, that's where we also run into similar problems with the users - we could/should create some kind of helper to make the user-switching the same.

Well, it is related, but depending on our solution, I’m not sure we’ll be able to translate that to the other issue. After all, there we’d need one user per role, more or less? (Hm, or just use one user and keep switching its roles during the indexing process …)
Anyways, once there is a solution, we can look into that.
Good point in any case, thanks!

Just ran into this issue on a views search page. The user.current_user_context service was getting the current user set from the search API. This would then load the user (doing a full entityTypeManager load of the user) to supply the user account. Blocks were then no longer rendering on the page that were limited to specific roles since the current_user was user 0. Using -2 as the uid didn't help either. I've created a merge request which uses the uid of the current user. Not sure if there are other negative consequences from doing this, but the site that we're using this on shouldn't have any negative consequences from doing this.

A more elegant solution to this would be to reset the current_user and user.current_user_context services after everything is rendered, but wasn't sure how to accomplish that.

I think this is a duplicate of #3110652: RenderedItem processor causes theme to think user is anonymous, please continue the discussion there.
(I’m not closing this as a duplicate yet so people with an interest in this issue have a better chance of seeing this comment.)