StatisticsPopularBlock breaks a page rendering when nid left orphan

Nice find, and yes, super simple fix.

1. +++ b/core/modules/statistics/src/Plugin/Block/StatisticsPopularBlock.php
   @@ -192,7 +192,7 @@ protected function nodeTitleList(array $nids, $title) {
        $nodes = $this->entityTypeManager->getStorage('node')->loadMultiple($nids);
   

   I think we should not load the nodes here, since this never gets used.

2. +++ b/core/modules/statistics/src/Plugin/Block/StatisticsPopularBlock.php
   @@ -192,7 +192,7 @@ protected function nodeTitleList(array $nids, $title) {
          $node = $this->entityRepository->getTranslationFromContext($nodes[$nid]);
   

   Instead maybe check the return value here?

@catch The initial node load is used as the param in getTranslationFromContext().

Well, instead of doing the workaround of iterating over the keys we could just do foreach ($nodes as $node), though, right?

The change in #8 is great, thanks for the suggestion @tstoeckler and the implementation @calmforce! This was already set to RTBC by @timmillwood before without it having tests, so setting this back to rtbc based on that.

I also think that writing a test for this would be really hard to get right.

+++ b/core/modules/statistics/src/Plugin/Block/StatisticsPopularBlock.php
@@ -192,8 +192,8 @@ protected function nodeTitleList(array $nids, $title) {
       $this->renderer->addCacheableDependency($item, $node);

I think the reason this was originally written looping over the $nids was to avoid overwriting $node, which this now does. I think we should probably rename the variable to $translation or something?

@catch - I believe you're wrong, $node has always been the translation.

That's not the point I was making, but got my selection in dreditor wrong so it wasn't clear maybe:

$node = $this->entityRepository->getTranslationFromContext($node);

There's no need to reassign $node here, we should use a different variable name, this wasn't a problem with the previous code.

Or in other words, $node was always the translation, now it starts as not being the translation, then becomes the translation.

@calmforce - @catch's view always outweighs the module maintainer. From knowing @catch for nearly 10 years his thoughts and ideas always end up being for the better.

So here's a patch adding the $translation variable.

So the patch in #8 is overwriting the value of $node in the foreach. Because the objects are by reference in PHP, this means that after that foreach loop, all of the array values would be the translations. Would need to double check, but it might even affect what gets loaded by Node::load() and Node::loadMultiple() later in the request, because that also returns by ref.

By assigning to a different variable name instead of overwriting as in #15, we're not changing the contents of the array in the foreach loop or anywhere else - i.e. $node remains exactly the same object.

An old issue that covered some of this is #154859: Document that cached entity objects are not necessarily cloned any more.

This looks like an edge case and the code has not changed. It is still looping on the passed in value $nids.

This needs a reroll and a test.

Attached Reroll patch against 10.0.x

#22 patch failed to apply, Need reroll.

Attached Reroll patch against 10.0.x with fixing failed testing patch issues.
Also take care #16 node suggestion part

Applied Patch against #24 in Drupal 10.x.1.

Statistics is approved for removal. See #3266457: [Policy] Deprecate Statistics module in D10 and move to contrib in D11

This is now Postponed. The status is set according to two policies. The Remove a core extension and move it to a contributed project and the Extensions approved for removal policies.

It will be moved to a contributed Statistics project once the project is created and the Drupal 11 branch is open.