Fix behaviour of entity_load when passed ids with a trailing dot

This is fixed and need to be tested by the community. Regards.

Works v well! Changing status to RTBC.

Tested on a clean install of D7

IIUC this is down to PHP's helpful type juggling.

(Example using PHP 7.4.3)

php > var_dump(is_numeric('5'));
bool(true)

php > var_dump(is_numeric('5.'));
bool(true)

php > var_dump('5.' == (int) '5.');
bool(true)

Any reason we couldn't change this to a strict comparison === instead of adding another test with a cast and a function call?

php > $id = 5;
php > var_dump($id === (int) $id);
bool(true)

php > $id = '5.';
php > var_dump($id === (int) $id);
bool(false)

What does D8/9 do?

Looks like D9 may use the same comparison here:

  protected function cleanIds(array $ids, $entity_key = 'id') {
    $definitions = $this->entityFieldManager->getActiveFieldStorageDefinitions($this->entityTypeId);
    $field_name = $this->entityType->getKey($entity_key);
    if ($field_name && $definitions[$field_name]->getType() == 'integer') {
      $ids = array_filter($ids, function ($id) {
        return is_numeric($id) && $id == (int) $id;  // is this effectively the same thing as we're looking to fix in D7?
      });
      $ids = array_map('intval', $ids);
    }
    return $ids;
  }

https://git.drupalcode.org/project/drupal/-/blob/9.4.0-alpha1/core/lib/D...

So does D9 have the same problem?

The IS mentions PostgreSQL as well as MySQL and the comment for \DrupalDefaultEntityController::cleanIds mentions:

   * The identifier sanitization provided by this method has been introduced
   * as Drupal used to rely on the database to facilitate this, which worked
   * correctly with MySQL but led to errors with other DBMS such as PostgreSQL.

Doing a bit of manual testing with MySQL initially I can confirm that e.g. these two URLs both successfully load node 1:

• http://d7.example.com/node/1
• http://d7.example.com/node/1.

Requesting the node with a different suffix to the id yields a 404 though - e.g.:

• http://d7.example.com/node/1_

I tried this with every non-alphanumeric character between 32-128 in the ASCII range and as far as I can see the dot is the only case where this happens. (Ignoring the characters that have special meanings in URLs such as & # ? etc..)

I believe that's down to the fact that the dot suffix is the only one that counts as numeric, presumably because it's treated as a decimal point even when there's nothing after it.

php > var_dump(is_numeric('5.'));
bool(true)

In D7, after the array_filter which lets '1.' through, the array of ids is mapped to the intval callback so '1.' becomes '1' which is why this works.

      if ($id_type == 'serial' || $id_type == 'int') {
        $ids = array_filter($ids, array($this, 'filterId'));
        $ids = array_map('intval', $ids);
      }

In other words, the case of the dot / decimal point after the id in the url seems to be unique.

Do we really want to "fix" this? On the one hand it's not uncommon for URLs to have a dot added accidentally e.g. in emails. However, it's technically probably duplicate content and would also likely be keyed differently by the page cache etc..

Confirmed that the same thing does not happen in D9; requesting node/1. yields a 404 as it should.

So I'd say we're okay to fix this as we see fit in D7.

I'd like to test the strict comparison approach (this patch does that), and I'd also suggest that we add a very basic test for this.

Here's a test only patch that should fail without an accompanying fix.

I'll have a look at why the previous patch is apparently breaking at least one test.

The test which is breaking with the $id === (int) $id patch is FilePrivateTestCase::testPrivateFile.

AFAICS that is because the test results in file_load_multiple() being called with an array of fids that just contains a 0.

Looks like that 0 gets passed to the array_filter callback as a string, which is where the strict comparison causes a problem:

php > $id = '0';
php > var_dump($id == (int) $id);
bool(true)

php > var_dump($id === (int) $id);
bool(false)

...whereas:

php > $id = '0';
php > var_dump($id == (int) $id && ctype_digit((string) $id));
bool(true)

So hmm...

I have a feeling passing the 0 as an entity id is a bug in itself, but perhaps we're better off minimising the risk of breaking things here and going with the implementation that doesn't break the existing test suite.

This patch combines #16 with the new test which we'd hope should pass.

One thing:

Can we have a quick comment of why the ctype_digit is there.

RTBC + 1, approved for Merge! Thanks all!

Good suggestion - I've added this comment on commit:

   protected function filterId($id) {
+    // ctype_digit() is used here instead of a strict comparison as sometimes
+    // the id is passed as a string containing '0' which may represent a bug
+    // elsewhere but would fail with a strict comparison.
     return is_numeric($id) && $id == (int) $id && ctype_digit((string) $id);
   }

We could add a todo to chase down the 0 as a string problem, but realistically I'm not sure there'd be much point.

Added a CR too as this is a change in behaviour.

Thank you everyone!