ContentEntityStorage::loadMultiple() returns the same for numeric entity IDs with leading zeroes and/or spaces

+++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
@@ -411,17 +411,20 @@ class SqlContentEntityStorage extends ContentEntityStorageBase implements SqlEnt
+    // Sanitize IDs. Before feeding ID array into buildQuery, check whether
+    // it is empty as this would load all entities.

Nitpick: 80 character wrapping not quite right.

Forgot to tag for testing.

Reviewed the patch from #4
Manual tested the patch on 9.4 on standard profile.
Steps followed from Issue summary
The new node can also be
- accessed at /node/01 and edited at /node/01/edit
- accessed at /node/001 and edited at /node/001/edit

Rerolled the path with Nitpick from #8

Confirming patch #10 works with 9.4.x (standard profile) and an existing 9.3.14 site:

Before applying the patch:
* Both /node/1/edit and /node/01/edit return the same node edit form for node #1.
* Both /node/1 and /node/01 return the same page for node #1.
* Both /user/1/edit and /user/01/edit return the same edit form for user #1.

After applying the patch:
* /node/1/edit and /user/1/edit work as usual.
* /node/01/edit and /user/01/edit now return error 404.
* /node/01 now returns error 404.

drush php with the patch:

Psy Shell v0.10.9 (PHP 8.0.17 — cli) by Justin Hileman
(Drupal 9.3.14)
>>> \Drupal\node\Entity\Node::loadMultiple(['1']);
=> [
     1 => Drupal\node\Entity\Node {#7216
       +in_preview: null,
     },
   ]
>>> \Drupal\node\Entity\Node::loadMultiple(['01']);
=> []
>>> \Drupal\node\Entity\Node::loadMultiple([' 1']);
=> []
>>> \Drupal\node\Entity\Node::loadMultiple([' 01']);
=> []
>>> \Drupal\node\Entity\Node::loadMultiple([1]);
=> [
     1 => Drupal\node\Entity\Node {#7216
       +in_preview: null,
     },
   ]
>>>

This could use some test coverage.

Sending a test-only patch which is expected to fail and the total patch.
I'm not sending an interdiff this time because I didn't change anything on SqlContentEntityStorage.php so the test-only patch is the inderdiff.

Edit: I'm not sure if this test is good enough as it doesn't cover cases like ' 01' or ' 1'.

I believe I've came up with a better test.
This time I assert using the loadMultiple() itself and I included the cases with space in the id.
Again sending a test only patch, which should fail and the complete patch.

Thanks! Removing tests tag.

Verified this on Drupal 10.1

Went to a URL node/0009 and it loaded the node/9 page
Applied patch
And now I get a page not found

+++ b/core/modules/node/tests/src/Functional/NodeCreationTest.php
@@ -117,6 +117,28 @@ public function testNodeCreation() {
+    $this->drupalGet('node/add');
+    $this->assertSession()->statusCodeEquals(200);
+    $this->assertSession()->addressEquals('node/add/page');
+    // Create a node.
+    $edit = [];
+    $edit['title[0][value]'] = $this->randomMachineName(8);
+    $this->drupalGet('node/add/page');
+    $this->submitForm($edit, 'Save');

This doesn't need to be a functional test, we should create the node via the Node::create API and move it to a Kernel test

Thanks for working on fixing this, which I discovered by chance a few months ago. It looks like the theme engine can be bypassed with this method.

Will it be back-ported to Drupal 7, which is also affected?

Not reproducible in 11.x branch. Running the following code produces the expected output shown in the attached screenshot.

dsm(\Drupal\node\Entity\Node::loadMultiple([' 1']));
dsm(\Drupal\node\Entity\Node::loadMultiple(['1']));
dsm(\Drupal\node\Entity\Node::loadMultiple(['01']));
dsm(\Drupal\node\Entity\Node::loadMultiple([' 1']));
dsm(\Drupal\node\Entity\Node::loadMultiple([' 01']));

May be good to least have test coverage around this