PostgreSQL: PDOException:Invalid text representation when attempting to load an entity with a string or non-scalar ID

This is not a DB issue. It's a problem with the menu system I suspect. Passing a string to an int field is supposed to fail. :-)

Marked #1041400: Error with PostgreSQL upon saving blog entry as a duplicate of this issue. I think this actually might be a bug with the entity API. This is caused by running node_load('a-string') or user_load('a-string'). I don't think the menu system is responsible for making sure $nids or $uids are casted correctly prior to calling, but we also usually take an anti-prevent-stupid-code stance, so I'm not sure what we need to be doing here.

Enforcing an int in the _load() functions would also help to prevent the not-so-helpful array_flip() errors you get when doing something like user_load(array('uid' => 1)).

A simple cast might not be the desired way to resolve this, though, because user_load('whatever') would be casted to 0 which would load the anon user. Other entities don't have an entity with id 0.

This can't be fixed in the menu system, since the values passed to _load functions do not always have to be integers (load by product code or machine name is quite a thin wrapper and quite useful).

I think that the right approach is remove non-integer values from the list of ids inside the entity load function before attempting the load - since they wouldn't be found in the list of integer ids anyway. This will result in 404 for non integer ids where there should be integer ones, which sounds correct to me.

Attached patch implements this change.

Component changed to base since entity isn't an option in the menu. Is that correct?

+++ includes/entity.inc	4 Feb 2011 07:37:32 -0000
@@ -170,6 +170,17 @@ class DrupalDefaultEntityController impl
+    // values. ¶
+    foreach ($ids as $key => $value) {
+      if ($value != (string) (int) $value) {
+        unset($ids[$key]);
+        trigger_error('Non-integer entity id \''.$value.'\'');
+      }
+    } ¶

- Some trailing whitespaces..
- I don't think we usually use trigger_error, if at all, we would directly write it to watchdog(). But I'm not sure about that either.
- The problem with both is that no new strings can be added, string-freeze... So unless there is a string somewhere that we can re-use here, we can't add any kind of log/display message.

Powered by Dreditor.

Updated patch attached.

Trailing whitespace cleaned up. The message has been removed - I'll change the version up to 8.x and add the message in once the fix has been committed.

The foreach needs to be wrapped in an is_array($ids) block.

Couldn't this be simplified to just.

  /**
   * Implements DrupalEntityControllerInterface::load().
   */
  public function load($ids = array(), $conditions = array()) {
    $entities = array();

    // Clean the $ids array to remove non-integer values.
    if (is_array($ids)) {
      $ids = array_filter($ids, 'is_numeric');
    }
    ....
  }

It is a gi-go short of issue, so I don't think a warning is required, particularly as MySQL silently fails anyway.

A quick patch created by hand:

Reverted from using is_numeric() as a callback, as I'm not sure how Postgres handles float to integer casting. If the ctype library is added as a requirement, it would be great to replace this with:

    if (is_array($ids)) {
      $ids = array_filter($ids, 'ctype_digit');
    }

I would expect this library to be much faster than callbacks or manual iterations for these sorts of tasks.

OK, my first git patch.

sub

Couldn't we just use array_filter instead of foreach? Unless its slower, but I doubt that, and its much cleaner.

#13: 1003788-faster-implementation-of-sanity-checking.patch queued for re-testing.

Bumping to D8 (patches now need to go against that)

#13: 1003788-faster-implementation-of-sanity-checking.patch queued for re-testing.

Why is the patch allowing null strings to go through, is that intentional?

mysql> select nid from node where nid='';
Empty set (0.00 sec)

psql> select nid from node where nid='';
ERROR: invalid input syntax for integer: ""

NULL means that the $conditions should be used to select the entities, it is not passed to the database.

In my case (D7 with PgSQL), I'm seeing the NULL is getting passed to the database:

Invalid text representation: 7 ERROR: invalid input syntax for integer: ""

if I add a check to that patch to unset where $value == '' too then my problems goes away but I'm by no means an expert and don't know what I'm doing.

A slight change to just array_filter() could resolve this. But that means that entities can never have 0 as an array key, which could be bad. Both MySQL & PgSQL start from 1 for AUTOINC, but do all db behave this way???

Haven't access to any dev tools, but the changes to #13 would be...

+    // Clean the $ids array to remove non-integer values that can be passed
+    // in from various sources, including menu callbacks.
+    if (is_array($ids)) {
+      $ids = array_filter($ids);
+    }
+

The attached patch is from a notepad edit.

This patch would enforce numeric entity ID. There is no documented requirement that entity IDs must be numeric in all cases in core. I think it would be a good idea to restrict to numeric, but we have never documented that.

See #1027908: entity_load() should return entities keyed by ID, not name for an on-going discussion about numeric vs. non-numeric entity IDs.

Given that, who is responsible for making sure that only id's are passed to those entity types for which the entity id *is* an integer? the caller? (menu system) , the database layer?

Given that, who is responsible for making sure that only id's are passed to those entity types for which the entity id *is* an integer? the caller? (menu system) , the database layer?

Drupal core schema uses integers as the entity primary key. Just have a look at a D7 database. It maybe is not documented, but impossible to use anything else.

subscribe.

If we don't *enforce* IDs are numeric now in core, rather than leaving it as an assumption, then we get crap like this. Core should not be held hostage to a poor decision made by the Entity API module.

Right the failing test is:

    // Test always denied access to the anonymous user contact form.
    $this->drupalGet('user/0/contact');
    $this->assertResponse(403);

Since the page not found is still a valid response, all that needs to change is to update this to get the big green light :)

    // Test always no access to the anonymous user contact form.
    $this->drupalGet('user/0/contact');
    $this->assertResponse(404);

When I apply the patch from 23, the error goes away, but I cannot run drush from the command line without running as user 1 (-u 1 option). Here's the error I get when I try to clear cache:

-sh-3.2$ drush cc all
Could not login with user ID #0. [error]
Command cache-clear needs a higher bootstrap level to run - you will [error]
need invoke drush from a more functional Drupal environment to run
this command.
The drush command 'cc all' could not be executed. [error]

-sh-3.2$ drush cc -u 1 all
'all' cache was cleared [success]
-sh-3.2$

If I comment out the patched lines, drush works fine:

-sh-3.2$ drush cc all
'all' cache was cleared [success]
-sh-3.2$

It's not that big a deal to have to use user 1, but is this indicative of something bigger?

Maybe this will require more than just changing the simpletest? Or should I bring this up to the Drush team?

#11: entity-load-text-filter-1003788-11.patch queued for re-testing.

This one should allow 0 to pass through, although it lacks the speed of the other methods (nothing has been benchmarked AFAIK). So there should be no change to any APIs, and Drush should run ok.

Alan, it's strange. When I actually deployed this patch, I was still getting the PDOException error. So I checked and it turns out that I was deploying #11. Then I went and deployed the patch from #23 and I no longer get the error (patch successful) and now it seems that drush is working just fine. Not sure why it wasn't working with user 0 before.

Is the patch in #11 successfully preventing the error for you? I'm getting the error when I install the Revisioning module on a Redhat machine with Postgres and php 5.3 installed. But it seems that #23 is working fine, but fails the simpletest...?

I should add that I'm on 7.2 core.

Man, going crazy...now that I re-applied the #23 patch (I re-applied 11 just to confirm) I'm getting my drush errors again. This is like hitting a moving target.

Sounds like something is being cached. Try #23 for a couple of hours (if you can) and check the results. If that fails, then apply #11, wait a couple of hours and try again. Sorry, I can not test any drush functionality.

Subscribing.

+++ b/includes/entity.inc
@@ -159,6 +159,12 @@ class DrupalDefaultEntityController implements DrupalEntityControllerInterface {
+    // Clean the $ids array to remove non-integer values that can be passed
+    // in from various sources, including menu callbacks.
+    if (is_array($ids)) {
+      $ids = array_filter($ids);
+    }

This is entirely incorrect and obviously leads to the bug in #31, only the earlier patch was right, though I'm not sure whether it really does what it's supposed to do without side-effects:

+++ b/includes/entity.inc
@@ -159,6 +159,12 @@ class DrupalDefaultEntityController implements DrupalEntityControllerInterface {
+    // Clean the $ids array to remove non-integer values that can be passed
+    // in from various sources, including menu callbacks.
+    if (is_array($ids)) {
+      $ids = array_filter($ids, 'intval');
+    }

Lastly, I think it's 1) too late for this change, and 2) not the right decision and direction in a long-term perspective.

There are no steps to reduce the bug here, the patch will magically ignore invalid arguments, not major, possibly won't fix.

Steps to reproduce:
1. Go to 'node/invalid-node' on PostrgreSQL

The patch from #11 does not filter integer 0's, passes current tests and doesn't upset contrib.

+    // Clean the $ids array to remove non-integer values that can be passed
+    // in from various sources, including menu callbacks.
+    if (is_array($ids)) {
+      foreach ($ids as $index => $value) {
+        if ($value !== '' && (!is_numeric($value) || intval($value) != $value)) {
+          unset($ids[$index]);
+        }
+      }
+    }

There are contrib bugs that cause ids of null and false to be passed in, this would hide those no?

I'd be fine with node_load() casting to int though.

intval is not correct - we need to use is_numeric()

intval means we are still loading an entity based on a garbage string :

php -r 'echo intval("32flavors");'
32

is_numeric() allows any numerical string and hexadecimal value, so that it atleast as wrong ;) For example, +0123.45e6 and 0xFF are are both valid values for is_numeric().

Then I think what you want is

if (!empty($id) && ($id == (int) $id)) {

or well, negated:

if (empty($id) || !($id == (int) $id)) {

so we cannot use the ctype functions?

I think #11 is not sufficient - we need this comparison:

(string) $value != (string) intval($value))

The suggestion from Sun:

$id == (int) $id

doesn't work because apparently PHP converts them both to int to compare.

Also this patch adds a couple assertions to the 404 test.

I think this will break the condition if $ids = FALSE? Running (array) cast on FALSE converts it to array(0 => FALSE). This will then fail the following code:

    ...
    $passed_ids = !empty($ids) ? array_flip($ids) : FALSE; <-- Will no longer be FALSE
    ...
    if ($ids === FALSE || $ids || $revision_id || ($conditions && !$passed_ids)) {
      ...
    }
    ...

Re-rolled as entity has moved into it's own module.

As per #46 and #47, php still casts back to numeric when comparing two numeric strings. Try, '42' == '42.00'. You have to use an identity(===) test to get around it. I changed one of the tests to show what I mean.

#48, Dave is right, the array casting messes up the special case of passing in FALSE.

The empty() check was added in to avoid trying to load a 0.

Fixed failed test. There still needs to be a decision if ids should ever be zero.

Not really able to review the code here - but I can say that the applied patch works fine on a 7.8 install (well - applied the changes from the patch in #51 but against includes/entity.inc as I don't yet have entity as an own module).

This patch is causing issues with drush.

Without the patch - drush is running fine.

With the patch - it fails to load user 0.

Drush is calling user_load(0) - which comes thru to this loop:

    foreach ($ids as $key => $id) {
        if (empty($id) || ((string) $id !== (string) (int) $id)) {
          unset($ids[$key]);
        }
      }
    }

empty($id) for $id = 0 returns true so we unset the passed in ID.

I found this bug when trying to use more than one token in pathauto (I found the a duplicate of this bug via Google, and it directed me here):

PDOException: SQLSTATE[25P02]: In failed sql transaction: 7 ERROR: current transaction is aborted, commands ignored until end of transaction block: SELECT column_name, data_type, column_default FROM information_schema.columns WHERE table_schema = :schema AND table_name = :table AND (data_type = 'bytea' OR (numeric_precision IS NOT NULL AND column_default LIKE :default)); Array ( [:schema] => public [:table] => url_alias [:default] => %nextval% ) in drupal_write_record() (line 6887 of /includes/common.inc).

As a quick & dirty workaround until the real fix comes around, I created the attached SQL that allows for some of the token replacement to happen in the database. This allows Drupal to use zero to one tokens with pathauto, which gets around the bug.

Basically, if a pattern begins with "TRIGGER/", then the trigger will remove the "TRIGGER/" prefix and perform an additional substitution on the remainder of the url_alias.alias column. Currently, only node ID, title, and user name are supported, but this could be extended to work with other values if needed (assuming that you have used up the one token that pathauto can substitute).

Last two comments suggest work needed.

My SQL workaround is provided just in case this patch breaks something that I need or someone does not want to manually backport this patch (and possibly others) for their Drupal 7 install.

I am on Drupal 7.9, so I cannot test the patch as is, but I can confirm that by manually adding the DrupalEntityControllerInterface.load() part of the diff from #51 into includes/entity.inc on Drupal 7.9, and with other patches that I have added, the pathauto issue goes away. I can now use two tokens in pathauto for blog posts. I do not use Drush, so I cannot comment on that.

I attached a diff/patch of my current Drupal 7.9 install which includes some other patches (1302158) and a temporary workaround that may be unnecessary now. I am not suggesting that anyone should use this; I am just sharing what has worked for me.

Since the 7.x-1.0 release of pathauto includes the workaround, I reduced my Drupal 7.9 patch down to the attached file. It is simply a copy and paste of the essential change from #51 by twistor. It does not have the test cases.

Previously it wasn't clear to me that entity IDs needed to be numeric. I created a custom module (using the Entity API module) that defines a number of entities that uses strings for their IDs. Sounds like I'll need to change the module and convert to using integer IDs.

I suspect this will be a problem for other developers, some of whom have probably already released contrib modules that define entities with non-integer IDs. I'm not sure what the best way of mitigating this is (at least a change record?), but I thought it important to mention.

Sounds like I'll need to change the module and convert to using integer IDs.

Yes! While you can do this, it breaks almost any compatibility with core. For example, look at the field tables, these have integer columns for the entity id. So for fieldable entities, you must have integer primary keys. It doesn't take too much to do. The countries module "real" primary key is the char string ISO2, but an autoincrement column (cid) was added to support fields, etc.

For the record, it is documented that entity_id must be a numeric value (integer), for example in http://api.drupal.org/api/drupal/modules--system--system.api.php/functio...

id: The name of the property that contains the primary id of the entity. Every entity object passed to the Field API must have this property and its value must be numeric.

That's only for fieldable entities though, right? Is it a requirement for ALL entities?

It should be, but things do work without it (currently). Note that integration with other modules will almost certainly fail, ie: entity api, etc.

Got it. I've been using my (non-fieldable) entities with non-numeric IDs on a production d7 site for 3 months with Entity API, Rules, and Views without issue.

I'm not saying the change shouldn't be made, just that it be considered that this will be a core breaking change for some developers because it restricts the allowed values for entity IDs from the current documentation and behavior, "Fieldable entity IDs must be integers", to "All entity IDs must be integers".

After applying patch #57 on D7, I've the following error using drush 4.4:

Could not login with user ID #0. This is typically caused by importing a MySQL database dump from a faulty tool which re-numbered the anonymous user ID in the users table. See http://drupal.org/node/1029506 for help recovering from this situation.

For real menu system input checking, I offer #63390: Secure argument passing. Someone needs to revive it.

With the patch from #57 (based on #51), I get the following on Drupal 7.9 & Privatemsg 7.x-1.2:
Notice: Trying to get property of non-object in privatemsg_user_access() (line 320 of [path to site]/modules/privatemsg/privatemsg.module).

If I undo the patch from #57, the above error goes away, but then I need the workaround in #54 for pathauto functionality.

That's only for fieldable entities though, right? Is it a requirement for ALL entities?

No it isn't - it's a requirement for fieldable entities only. And it really should not be a general requirement, as this is needed in order to be able to deal with remote entities that do not have a numeric id.

Still, we can make the default controller to require numeric ids - what it assumes anyway.

After applying the patch#57 above, I wanted to report success in eliminating the warning messages that were present before:

For the record I have a 7.10 installation with several modules installed (all up to date) and was getting the
Warning: array_flip() [function.array-flip]: Can only flip STRING and INTEGER values! in DrupalDefaultEntityController->load() (line 178 of .../includes/entity.inc)

This happened whenever I try to add content, e.g.
Add Content
Article - 5X
Basic Page - 0X
Blog entry - 3X
Book page - 3X
Chat - 2X
Chat Room - 2X
Date - 4X
Forum topic - 4X
Gallery - 14X
Group Content - 2X
Poll - 2X

(I was getting the above warning message N times (NX) after clicking Add Content for the above content element)
This is when I am logged in as an authenticated user, and happened every time I tried to Add Content.

#57 worked for me but I'm wondering if there is a way to identify the module that is causing the error?

I've changed the patch from #57 to fix the drush "Could not login with user ID #0" error. Updated patch attached.

It seems like there is no consensuses on the use of zero, even though non-one has even considered negative values. Re-roll of #70 allowing any integer, and a test for 'node/-1' that could open new issues...

#57: 1003788-57-drupal7.patch queued for re-testing.

#57 is the patch that disallows 0 but allows -MAX_INT_SIZE to -1 and 1 to MAX_INT_SIZE (written prior to core restructure)
#71 allows any integer, bypassing issues where 0 is used as a db entity.

I agree with #63. The #71 patch essentially breaks loading [non-fieldable] entities with string ids - something which, up until now, has not been explicitly prohibited or discouraged in documentation. Right?

I'm not sure when [or if] consensus was reached in this matter, but maybe I missed a conversation on IRC or elsewhere. It would just be interesting to hear from more maintainers of modules using strings as entity ids and their thoughts on the subject. I don't actually know how many there are out there, but judging by some of the comments there ought to be a couple more than just Wysiwyg.

What should we do for Wysiwyg's editor profiles instead, where the format id is our only useful way to find/load a profile for a specific format? Add an extra [unused] profile_id field and load profiles using the db_* API only? How many modules - and how much in them - will this change impact?

UPDATE: Found #368082: Non-numeric object ids and revision ids via http://drupal.org/community-initiatives/drupal-core/fields, thought it might be relevant.

Is this issue really duplicated by #1102570: array_flip() [function.array-flip] issue in DrupalDefaultEntityController / entity.inc? People keep commenting there, the link between these issues is not obvious.

Attache a D7 version of patch #71, as we have a pressing need to fix this in Drupal 7 now.

We are running on postgres 9.1, Drupal 7.12, and can confirm it resolves our issue

Hope this gets into D8 shortly so it can be backported ASAP.

#74 suggests that entity ids should be allowed to be a string.

I think this leaves of with the following choices:
1) Only support numeric/digit ids.
2) Allow for string ids and fix every single broken module to accept or handle the strings as a possible entity id.
3) Write a new entity variable that defines what type of data is supported. That is, something like digit_only, then when this is true, enforce the digit only test.

EDIT:
as for choice #1, might I suggest casting to floats instead of integer or string, like such:

<?php
      foreach ($ids as $key => $id) {
        if (!is_numeric($id) || (float) $id !== round($id)) {
          unset($ids[$key]);
        }
      }
?>

0 should still be allowed by this suggestion.

Numeric ID's?! Hopefully nothing uses these!!! Floats would round differently on different systems, so you should never use these as IDs.

I get the feeling that the core team are leaning to string IDs too, but it would be nice to actually get a definitive decision on this. I'm sure that this issue would be irritating PostgreSQL users a lot.

As this directly affect PostgreSQL users using Drupal 7 core (without any additional contrib). It makes sense to me to leave DrupalDefaultEntityController to attempt to load non-numeric IDs and fix those occurrences in core that allow strings to be loaded as integers. In particular, the node system:

protected function buildQuery($ids, $conditions = array(), $revision_id = FALSE) {
  // Ensure that uid is taken from the {node} table,
  // alias timestamp to revision_timestamp and add revision_uid.
  $query = parent::buildQuery($ids, $conditions, $revision_id);
  $fields = &$query->getFields();
  unset($fields['timestamp']);
  $query->addField('revision', 'timestamp', 'revision_timestamp');
  $fields['uid']['table'] = 'base';
  $query->addField('revision', 'uid', 'revision_uid');
  return $query;
}

All that is needed is to add an additional line of code that filters the ids by those that are numeric. This seems a fair fix as the {node} table relies on nid to be numeric while other entity_id columns may not.

$ids = array_filter($ids, 'is_numeric');

Marked #1632458: Error occurring when visiting $path_drupal/user/edit as duplicate.

What does "node_load(123.456)" do? I would assume that PostgreSQL would also throw an error at this too.

While there is reference to some other related issues like doing this: user_load(array('uid' => 1)), etc, are getting resolved in the queues as being GIGO issues. The reported error is related to the menu loaders, so this would probably involve:

aggregator_feed_load
aggregator_category_load
comment_load
contact_load
node_load
taxonomy_vocabulary_load
user_load

But not taxonomy_term_load(), although I'm not sure what "user/123.123" would do

function taxonomy_term_load($tid) {
  if (!is_numeric($tid)) {
    return FALSE;
  }
  ...

And update the hook_entity_info() notes:

- * - fieldable: Set to TRUE if you want your entity type to accept fields being attached to it.
+ * - fieldable: Set to TRUE if you want to attach fields. Note: The Field API require unsigned integer entity keys.

I don't think #78 understood me.

By casting to floats, we are able to avoid php's "optimization" and ensure that we can confirm if something invalid such as a float is used. (Assuming we are going with the numeric id only approach).
By comparing the float casted input against a rounded input. we would get something like: if ((float)5.5 !== (float)5).
Which this test works.
What I am uncertain of is which performs better, converting to a string and back, or converting to a float and rounding?

And yes, https://example.com/node/5.5 does produce an error just like strings do.

Perhaps a combinations of :

$ids = array_filter('is_numeric', $ids);
$ids = array_map('intval', $ids);

In those cases where we expect an int ID?

Getting a very similar error with file components on webforms:

When a file component is not required and the webform is submitted without an uploaded file, you get the following error:

PDOException: SQLSTATE[22P02]: Invalid text representation: 7 ERROR: invalid input syntax for integer: "" LINE 4: WHERE (base.fid IN ('')) ^: SELECT base.fid AS fid, base.uid AS uid, base.filename AS filename, base.uri AS uri, base.filemime AS filemime, base.filesize AS filesize, base.status AS status, base.timestamp AS timestamp FROM {file_managed} base WHERE (base.fid IN (:db_condition_placeholder_0)) ; Array ( [:db_condition_placeholder_0] => ) in DrupalDefaultEntityController->load() (line 196 of /home/adamb/projects/drupal/includes/entity.inc).

Marked #1676214: PDOException with Entity Cache and file upload field submitted without a file as a duplicate of this

There's a perceived security issue here because a lot of scanners look for node/1.bak and think that the resulting 200 page is an indication of an insecure site. Tagging accordingly.

The same issue with userpoints module: #1446528: PostgreSQL error in transaction_load()

Marked another issue as a duplicate of this #1391328: postgresql error: invalid input syntax for integer

Same issue here... user one time pass with D7 with PgSQL gets pretty similar error:

http://my_site.com/user/reset/10/1353948070/r3pU5ScOErf2-1bPsqqvj-q9RHC0...

PDOException: SQLSTATE[22P02]: Invalid text representation: 7 ERROR: invalid input syntax for integer: "reset" LINE 4: WHERE (base.uid IN ('reset')) ^: SELECT base.uid AS uid, base.name AS name, base.pass AS pass, base.mail AS mail, base.theme AS theme, base.signature AS signature, base.signature_format AS signature_format, base.created AS created, base.access AS access, base.login AS login, base.status AS status, base.timezone AS timezone, base.language AS language, base.picture AS picture, base.init AS init, base.data AS data FROM {users} base WHERE (base.uid IN (:db_condition_placeholder_0)) ; Array ( [:db_condition_placeholder_0] => reset ) en DrupalDefaultEntityController->load() (línea 196 de /home/tabletas/public_html/includes/entity.inc).

Another duplicate: #1869306: PDOException:Invalid text representation when attempting to load a view with a string argument

Coming from the Views issue mention in #90. So, we still have non consensus on how to fix this issue.
- Remote entities is not always numeric so simply limiting them it to numeric types is not a good idea.
- Testing for is numeric will not catch things like node/343.3
- Writing fixes for all core *_load() neither sounds like a great idea.

Seems to me that the real underlying issue in the user/reset case (#89) is that it's trying to load a user in the first place. Shouldn't this just go to the user module's reset handler which then loads user 10?

If for some odd reason *all* paths are routed in such a way that it will always try to load the second argument, this really is a security issue, even with mysql. (one could imagine a timing attack to see whether the load succeeded, which results in an information leak)

I've written a module (currently still a sandbx) called Numeric Identified Entity: http://drupal.org/sandbox/fiasco/1786222
Its a simple wrapper module that allows you to enforce entities that have numeric identifiers to have numeric values passed to it before calling the load function.

Adding this module does fix this issue.

Kia Ora Josh

I'll take your code for a spin as soon as I find the time. Had a quick look at the code and it looks OK.

Encountered this again with #1920862: Rename custom_block.module to block_content.module having a custom custom.module along with core's custom_block.module makes custom_block_load() convert into an implementation of hook_block_load() and instead of IDs it gets passed nice block entity objects, which fails miserably on MySQL.

We have entity IDs that are strings in D8, so for D8 this is entire issue is a non-starter.

Moving down to 7.x. Although I'm not sure whether this is worth any fix.

Things still explode if you pass in a non-string and non-integer value, like an object.

My setup: Drupal 7.22 / PHP 5.3 / IIS 7.5 / SQL Server 2008

#76 fixed my issue. Before that patch, this is was I was experiencing with D7:

• After install, a new menu item for "Field Collection" never got created for me under Admin > Structure.
• If I tried to create one manually, I received this error: "The path 'field-collection' is either invalid or you do not have access to it."
• If I tried to get to it through Admin > Modules > Field Collection > Configuration, it linked to: http://mysite/drupal/admin/structure
• Even though I had deleted all field collections, several errors like these still appeared (as they also slowed down my site tremendously) in the "Recent log messages":
  PDOException: SQLSTATE[22018]: [Microsoft][SQL Server Native Client 11.0][SQL Server]Conversion failed when converting the nvarchar value '%/moderation' to data type int.: SELECT revision.[vid] AS [vid], base.[uid] AS [uid], revision.[title] AS [title], revision.[log] AS [log], revision.[status] AS [status], revision.[comment] AS [comment], revision.[promote] AS [promote], revision.[sticky] AS [sticky], base.[nid] AS [nid], base.[type] AS [type], base.[language] AS [language], base.[created] AS [created], base.[changed] AS [changed], base.[tnid] AS [tnid], base.[translate] AS [translate], revision.[timestamp] AS [revision_timestamp], revision.[uid] AS [revision_uid] FROM {node} base INNER JOIN {node_revision} revision ON revision.vid = base.vid WHERE ( ([base].[nid] IN (:db_condition_placeholder_0)) ); Array ( [:db_condition_placeholder_0] => %/moderation ) in DrupalDefaultEntityController->load() (line 196 of \mysite\drupal\includes\entity.inc).
• The only way that these errors go away is if I disable and uninstall the Field Collection module

Thank you so much wiifm for solving this issue for me. It's been a real pain.

I was trying to install commerce kickstart 7.x-2.6 in an Window Azure website and got the same error. As well as for the #98, the #76 fixed my issue.

Resetting status.

@jonathanmv, This issue is about Drupal as a whole, and hasn't been resolved yet.

Marked #1795918: Can only flip STRING and INTEGER values! as a duplicate of this issue.

Same issue with when revisionning module was activated on our setup => Drupal 7.22 / Postgresql 9.1.9
#76 did solve the issue.

Right, nearly 3 years and no progress. Maybe a pure sanity check on scalar values, leaving any other validation to the defining entity?

+    // Clean the $ids array to ensure non-empty scalar id's.
+    if (is_array($ids)) {
+      foreach ($ids as $key => $id) {
+        if (!is_scalar($id) || (empty($id) && (string)$id !== '0')) {
+          unset($ids[$key]);
+        }
+      }
+    }

To solve this the easiest solution might be to add some checks in *StorageController for the respective entity. That should allows us to have different restrictions for different entities. This should solve it for the nodes (and other int-based ids).

$ids = array_filter($ids, function($var) {
  return is_numeric($var) ? intval($var) == $var : FALSE;
});
$ids = array_map('intval', $ids);

This will filter out everything but integers and strings that can be converted to integers. Floats will be filtered out too.

Another solution would be to let the caller of the load function cast the parameters first, but that might bring in some bad developer experience and would also require more changes.

I wrote the Numeric Identified Entity module (which is still a sandbox) for Drupal 7 which is fix worth considering something similar for Drupal 8.

In essence, it introduces a pseudo entity class that overrides the default load method and parses the passed ids as numeric before passing it onto DrupalDefaultEntityController::load().

For Drupal 8 however, what about introducing another key to the entity_keys entity info definition that defines the datatype expected for the entity id?

Attached is a simple proof of concept patch for Drupal 8.x

Fixed syntax error in docs

(Drupal 7 note)
If using entitycache module, the changes from the patch #74 have to be also applied to entitycache.module (entityCacheLoad).

Hi!
any idea if this issue is going to be resolved? I'm having problems in D7 when saving nodes with attachments on postgresql. Error generated by pathauto (pathauto.inc:480) when trying to get menu item (menu_get_item() ) the cause is the same of this issue: is trying to load file entities with a string instead a integer id.

Patch sanitization made by #71 solved the problem but don't now if it is the best solution. If it is the best solution why it's not included in core yet.

Move this to critical. I ran all D8 tests locally on PG and >90% of the failing tests are caused by this issue! This needs fix ASAP.

108: 1003788-8.x-entity-id-datatype-support.patch queued for re-testing.

Rerolled
Great! Patch still passes. Can now please someone make a decision if this is RTBC or not after 3 years? It will make so much more tests pass on PostgreSQL.

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageController.php
@@ -98,6 +98,12 @@ public function __construct(EntityTypeInterface $entity_info, Connection $databa
+      $ids = array_filter($ids, 'is_numeric');

If any of $ids isn't numeric, we should still throw an exception here.

For Content Entities, we can now rely on the field definitions to figure the data type out, as we recently did for #1823494: Field API assumes serial/integer entity IDs, but the entity system does not. A bit more complicated to check for, but no need to introduce another key.

This will also need a integer and a string key test, which might be possible with unit tests, similar to the other issue, depends on how well injected the storage controller already is.

While running more tests locally it seems that the exactly same PDOException happens more widely even outside node context. It seems that maybe a more "robust" solution to this is needed. I would suggest to wait for a working PG Testbot to identify all exceptions related to this one before going further.

I know this isn't the place, just giving feedback, don't shoot the pianist that doenst speak php

We have exactly the same issue (the same error) after installing drupal7 on debian wheezy postgresql, and then deploying the https://drupal.org/project/drupal_wiki distribution.
(drupal 7.14-2)

A fix that seems to work is to (just testing it) change the
"protected function buildQuery($ids, $conditions = array(), $revision_id = FALSE)" function
in entity.inc

if ($ids) {
// origineel
// $query->condition("base.{$this->idKey}", $ids, 'IN');
// nieuw
$rij;
foreach ($ids as $ifield => $ivalue) {
$rij[$ifield]=(int) $ivalue;
}
$query->condition("base.{$this->idKey}",$rij, 'IN');
}

solution on #118 works for me..

https://drupal.org/comment/8651081#comment-8651081

Yepp, #118 works for me too on Drupal 7.26.
Seems like Drupal handles bugs like this like KDE :D A few years go by, noone cares, that's life ... >:)

Here's a rather quick patch that works for both nodes (/node/invalid) and users (/user/invalid) rewritten to use the suggestion in #116. It currently only checks integers.

Added a small null-check.

Just a note this is a potential DDOS vector. If you have load balancers that are checking for 500 errors to determine bad web heads, just visiting these paths can mean all marked as bad/endless cycling.

So this being a postgres-only bug would explain why the test isn't failing. The patch needs a reroll (it didn't apply before the PSR-4 change). Also did a quick code review (both coding standards and architecture):

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityDatabaseStorage.php
   @@ -1455,4 +1459,33 @@ static public function _fieldColumnName(FieldConfigInterface $field, $column) {
   +   * Verifies that the specified entity ids are of the correct data type for
   +   * the entity type.
   

   Shorten this to 80 characters; also "IDs" rather than "ids". https://drupal.org/node/1354#functions

2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityDatabaseStorage.php
   @@ -1455,4 +1459,33 @@ static public function _fieldColumnName(FieldConfigInterface $field, $column) {
   +   *   The entity ids to verify.
   

   IDs. :)

3. +++ b/core/lib/Drupal/Core/Entity/ContentEntityDatabaseStorage.php
   @@ -1455,4 +1459,33 @@ static public function _fieldColumnName(FieldConfigInterface $field, $column) {
   +    $entity_manager = \Drupal::entityManager();
   

   Do we not have the entity manager injected? If we don't already, then maybe this method shouldn't be on this class? Actually, I'm thinking maybe this method doesn't belong on this class in any case. Validation of ID values would seem to be the entity type's domain, not the storage's. Or am I mistaken?

4. +++ b/core/lib/Drupal/Core/Entity/ContentEntityDatabaseStorage.php
   @@ -1455,4 +1459,33 @@ static public function _fieldColumnName(FieldConfigInterface $field, $column) {
   +          if (ctype_digit($id)) {
   +            $ids[$index] = intval($id);
   +          } else {
   +            unset($ids[$index]);
   +          }
   

   Else should be on its own line (not cuddled). https://drupal.org/coding-standards#controlstruct

5. +++ b/core/lib/Drupal/Core/Entity/ContentEntityDatabaseStorage.php
   @@ -1455,4 +1459,33 @@ static public function _fieldColumnName(FieldConfigInterface $field, $column) {
   +    return $ids;
   

   The method has a return value (the sanitized list of IDs) but this is missing from the docblock.

6. +++ b/core/modules/system/lib/Drupal/system/Tests/System/PageNotFoundTest.php
   @@ -35,6 +35,12 @@ function testPageNotFound() {
   +    // Test some invalid ids
   

   This comment isn't very specific (also should end in a period, and the "ids" bit).

7. +++ b/core/modules/system/lib/Drupal/system/Tests/System/PageNotFoundTest.php
   @@ -35,6 +35,12 @@ function testPageNotFound() {
   +    $this->drupalGet('node/invalid');
   +    $this->assertResponse(404);
   +    $this->drupalGet('user/invalid');
   +    $this->assertResponse(404);
   +
   

   Red flag here because this adds a dependency on these two modules. User is a required module, but node is not.

   Since the fix looks like it's going to be related to the entity system, the test coverage should probably use the test entity type and be in the entity tests somewhere.

@catch, so yet another reason to move this issue forward? :)

Just to summarize, we have:

• The security issues in #125, #86
• The Postgres users who want all tests to pass on testbot. Apparently fixing this issue would greatly reduce failed Postgres tests: comment #111, #2157455: [Meta] Make Drupal 8 work with PostgreSQL or remove support from core before release and #710858: Meta issue: fix the remaining PostgreSQL bugs
• The actual error showing up in real life situations:

#1041400: Error with PostgreSQL upon saving blog entry
#1022648: PDOException - Invalid input syntax for integer: "foobar"
#1102570: array_flip() [function.array-flip] issue in DrupalDefaultEntityController / entity.inc
#1135906: Installation with MSSQL 2008 Enterprise
#1225264: "ORA-01722: invalid number" error at many places
#1343934: Meta Tags & WYSIWYG Incompatibility?
#1391328: postgresql error: invalid input syntax for integer
#1444514: Attempting to access path 'node/noninteger' fails with database error
#1446528: PostgreSQL error in transaction_load()
#1565952: Request to node/1.json Returns 500 Service unavailable
#1632458: Error occurring when visiting $path_drupal/user/edit
#1606320: Database/SQL errors when non-numeric id is used
#1676214: PDOException with Entity Cache and file upload field submitted without a file
#1744506: RestWS node/1.json does not work with postgres (setting the Accept header and accessing node/1 does work)
#1795918: Can only flip STRING and INTEGER values!
#1869306: PDOException:Invalid text representation when attempting to load a view with a string argument
#1920862: Rename custom_block.module to block_content.module
#1963198: Unable to administer modules: PDO Error
#1999754: Postgresql error when accessing node/1.rdf
#2001350: [meta] Drupal cannot be installed on PostgreSQL
#2093779: Conversion failed when converting the nvarchar value 'admin' to data type int - MSSQL Incompatibility?

Can anyone chime in on whether #124 is the way to go and now it's just a matter of getting the patch right? Ie. filtering out invalid entity ID's on the relevant fieldable entities depending on the entity ID data type definition, with (as catch asked for in #115) an extra exception on invalid entity IDs?

Personally it looks good to me, how do we deal with this in a D7 backport though? In D8 we can get the correct data type from the field definition but in D7 we can't, so this check may just have to be only on the NodeController and UserController?

Back when this was a D7 issue it seems fago was OK with doing some validation in the core entity controller (#67) that would not necessarily be in other entity controllers (ie. non-fieldable ones), and I think only sun expressed some concern with requiring integers (#39: possible side-effects, "not the right direction longer-term", "too late for this change", #96: "not sure whether this is worth any fix" [in D7]).

Note that ctype_digit() can not be used here, as in the last patch, as it is for testing for numeric characters (not variable types):

i.e.

$integer = 42;
# Prints "n" as ASCII 42 is the * character.
print ctype_digit($integer) ? 'y' : 'n';  

So imho it is better to use the integer comparisons from earlier patches, but this would work:

$integer = 42;
# Prints "y" as this is a string of numeric characters.
print ctype_digit("$integer") ? 'y' : 'n';  

This is the patch from @JimmyAx in #124, incorporating the feedback by @xjm and @Alan D. The testbot people in Austin are working on getting Postgres to run on a Docker image, as soon as they do (which should be shortly) I'll post Postgres results from this patch along with a test-only patch.

As to point #3 by xjm, I wasn't sure where else to put the ID validation as it's still a storage-specific fix.

131: 1003788-8.x-entity-id-131.patch queued for re-testing.

Patch 131 looks great, and works as advertised. I've tested it, and after having followed this issue for a number of years, I think I can finally say it's ready for prime time.

+++ b/core/lib/Drupal/Core/Entity/ContentEntityDatabaseStorage.php
@@ -131,6 +135,30 @@ protected function doLoadMultiple(array $ids = NULL) {
+   */

It's a nitpick, but this only sanitizes IDs to integers, it doesn't sanitize string IDs - could use more explanation about why it's needed (i.e. the postgres error).

Looks like a 81 character line sneaked in, corrected patch attached.

Further refinements from @catch feedback

Looks good if green.

Thanks. Let me just remove that 81 character line that sneaked back in in the previous patch...

And a spelling mistake (lead -> led)

Committed/pushed to 8.x, moving to 7.x for backport.

7.x backport -- there are no field definitions so I used the schema instead, does that seem OK?

Um, but why was this added?

+        case 'char':
+        case 'varchar':
+          $ids = array_filter($ids, 'is_string');
+          $ids = array_map('strval', $ids);
+          break;

Wouldn't any scalar variable be considered a string in a loosely typed language like php? Or to put it another way, has there been a single reported error because of this? Most errors seem to come from menu wildcards which are always strings.

If this goes in (and maybe to consider for D8), is_scalar() could be safer.

Also a slight API change, I am not sure if their could be ramifications.

is_string('23') = bool(true)
is_string(23) = bool(false)

I maintain one module that has string IDs and this should be ok.

You're right. Let's take that out and fix the docs instead to clarify it only ensures integers are valid. Reverted.

I had planned to open a quick follow-up for this, but now that it's been reverted let's please fix the following:

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityDatabaseStorage.php
   @@ -349,6 +353,39 @@ protected function doLoadMultiple(array $ids = NULL) {
   +    $keys = $this->entityType->getKeys();
   +    $bundle = $keys['bundle'] ? $keys['bundle'] : $this->entityTypeId;
   ...
   +    $definitions = $this->entityManager->getFieldDefinitions($this->entityTypeId, $bundle);
   

   $bundle and $keys are unnecessary, just use $this->entityManager->getBaseFieldDefinitions($this->entityTypeId);
   It's not supported to change the type of the ID field per bundle.

2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityDatabaseStorage.php
   @@ -349,6 +353,39 @@ protected function doLoadMultiple(array $ids = NULL) {
   +    $info = $this->entityManager->getDefinition($this->entityTypeId);
   +    $id_definition = $definitions[$info->getKey('id')];
   

   $info is unnecessary, just use $this->entityType->getKey('id')

No break required :)

+      break;
+    }

I have tested this locally, guessing this can go back to RTBC as the code is essentially the same?

I'm adding one additional test case. I believe that the current patch will accept /node/123 as well as /node/123.123 to be the same node. This is different from the current behavior of not accepting /node/123.123. I believe my patch in #124 fixes that. (It still needs to be updated to incorporate the review of it and change if (ctype_digit($id)) { in it to be if (is_string($id) && ctype_digit($id)) { to account for non-strings.) Thoughts?

OK, I think what is needed is to define what valid integers are and then write an array_filter callback and then then test for this.

To me, these would be a core set of integers that are true PHP integers or an numeric string with optional - prefix.

$ids = array(-1, 0, 1,  '-1','0', '1', 0123,  -0x1A, 0x1AFC, -0b111, 0b101);
$invalid_ids = array('0123', '-0x1A', '0x1AFC', '-0b111', '0b101', 'a', 'f', FALSE, TRUE, NULL, -1.0, 0.0, 1.0, '32acb');

function is_integer_value_1($id) {
  return is_int($id) || ctype_digit($id);
}
function is_integer_value_2($id) {
  return ((string) $id === (string) (int) $id);
}
function is_integer_value_3($id) {
  return is_int($id) || (is_string($id) && ctype_digit(ltrim($id, '-')));
}
function is_integer_value_4($id) {
  return ctype_digit(ltrim((string)$id, '-'));
}
function is_integer_value_5($id) {
  return filter_var($id, FILTER_VALIDATE_INT) !== FALSE;
}

print "\n there are " . count($ids) . " valid integers\n";
print "\n1\n";
print "\n there are " . count(array_filter($ids, 'is_integer_value_1')) . "valid integers\n";
var_export(array_filter($ids, 'is_integer_value_1'));
var_export(array_filter($invalid_ids, 'is_integer_value_1'));

print "\n2\n";
print "\n there are " . count(array_filter($ids, 'is_integer_value_2')) . " valid integers\n";
var_export(array_filter($ids, 'is_integer_value_2'));
var_export(array_filter($invalid_ids, 'is_integer_value_2'));

print "\n3\n";
print "\n there are " . count(array_filter($ids, 'is_integer_value_4')) . " valid integers\n";
var_export(array_filter($ids, 'is_integer_value_3'));
var_export(array_filter($invalid_ids, 'is_integer_value_3'));

print "\n4\n";
print "\n there are " . count(array_filter($ids, 'is_integer_value_4')) . " valid integers\n";
var_export(array_filter($ids, 'is_integer_value_4'));
var_export(array_filter($invalid_ids, 'is_integer_value_4'));

print "\n5\n";
print "\n there are " . count(array_filter($ids, 'is_integer_value_5')) . " valid integers\n";
var_export(array_filter($ids, 'is_integer_value_5'));
var_export(array_filter($invalid_ids, 'is_integer_value_5'));

is_integer_value_3() correctly passes all valid integers (by my definition), and has only one false hit ('0123');

is_integer_value_1() skips '-1', with 1 invalid false positive

Other functions allow all of the valid integers through, but:

is_integer_value_2() has 4 false positives
is_integer_value_4() has 5 false positives
is_integer_value_5() has 4 false positives

@JimmyAx (#155) that is a good point, cleanIds() does currently convert 123.123 into 123, which I don't think we want, but the test you added may be wrong as it's actually passing (with the patch going to node/123.123 will give you a page not found even if node/123 still exists)

@Alan D. this one makes sense as opposed to is_numeric (which was used in most of the other patches in this issue)

function is_integer_value_3($id) { return is_int($id) || (is_string($id) && ctype_digit(ltrim($id, '-'))); }

@Alan D., I just read the comment underneath the code you posted. Just so we have no false positives at all (and to make it even more unreadable :), what about:

function is_integer_value_6($id) { return is_int($id) || (is_string($id) && ctype_digit(ltrim($id, '-')) && $id[0] !== '0'); }

edit: nevermind, that will still throw out the string '0'. It'd need to be this bizarreness:

function is_integer_value_7($id) { return is_int($id) || (is_string($id) && ctype_digit(ltrim($id, '-')) && ($id[0] !== '0' || $id === '0')); }

So something like this?

The previous patch had the issue in pointed out in #158, revised patch attached.

I really couldn't believe how badly PHP handles integer test!

Is it worth having a real function to handle this? I have seen other instances of integer checks in the past in various places, and most used something similar to "(string) $id === (string) (int) $id"

Possible addition:

/**
 * Helper function to test for integer values.
 *
 * @param mixed $value
 *   Value to test.
 * @return bool
 *   returns TRUE if the value is a true PHP integer or a decimal string representation 
 *   of an integer that can be cast safely to an integer using intval().
 */
function is_integer_value($value) {
  return is_int($id) || (is_string($id) && ctype_digit(ltrim($id, '-')));
}

Note the wording, it would allows 0 padded, as long as intval() correctly works. The only failures I saw were with the octal sting version, and I think it would be ok if we assumed that we would treat it as a zero padded decimal string.

Manual testing script again.

$ids = array(-1, 0, 1,  '-1','0', '1', 0123,  -0x1A, 0x1AFC, -0b111, 0b101);
$invalid_but_acceptable = array('0123', '00123', '000123', '-0123', '-00123', '-000123',);
$invalid_ids = array( '-0x1A', '0x1AFC', '-0b111', '0b101', 'a', 'f', FALSE, TRUE, NULL, -1.0, 0.0, 1.0, '32acb');

var_export(array_filter($ids, 'is_integer_value'));
var_export(array_filter($invalid_but_acceptable, 'is_integer_value'));
var_export(array_filter($invalid_ids, 'is_integer_value'));

foreach ($ids as $value) {
 print "\nThe value " . $value . " was cast to " . intval($value) . ".\n";
}
foreach ($invalid_but_acceptable as $value) {
 print "\nThe value " . $value . " was cast to " . intval($value) . ".\n";
}

Re: patch
It is better to clearly distinguish this better: "A || B && C" to "A || (B && C)" or I think that is what I think is happening... ;)

Did you try simply with is_numeric($i) && $i == (int)$i) ?

It yeilds false positives only for -1, 0 and integers written as floats with a 0 decimal. And those values are valid integers they just have invalid business meaning.

$invalid_but_acceptable = array('0123', '00123', '000123', '-0123', '-00123', '-000123',);
$invalid_ids = array( '-0x1A', '0x1AFC', '-0b111', '0b101', 'a', 'f', FALSE, TRUE, NULL, -1.0, 0.0, 1.0, '32acb');

function is_int_test($a) {
    return is_numeric($a) && $a == (int)$a;
}


echo "valid: ";
foreach ($invalid_but_acceptable as $i) {
  echo "  ", $i, " => ", is_int_test($i), "\n";
}

echo "invalid: ";
foreach ($invalid_ids as $i) {
  echo "  ", $i, " => ", is_int_test($i), "\n";
}

@Alan D: having seen all this, if we were to do an integer test without any false positives/negatives at all, what snippet would you use?

Turning it into a separate function and replacing other faulty integer tests elsewhere could be a separate issue, with whatever we come up with here as a start.

@pounard
Works for me, effectively does the intval() check in the filter. :)

Updated function and example tests:

function is_integer_value($value) {
  return is_numeric($value) && $value == (int)$value;
}
$ids = array(-1, 0, 1,  '-1','0', '1', 0123,  -0x1A, 0x1AFC, -0b111, 0b101);
$invalid_but_acceptable = array('0123', '00123', '000123', '-0123', '-00123', '-000123', -10.0, -1.0, 0.0, 1.0, 10.0, -10.00, -1.00, 0.00, 1.00, 10.00);
$invalid_ids = array( '-0x1A', '0x1AFC', '-0b111', '0b101', 'a', 'f', FALSE, TRUE, NULL, '32acb', 123.123, 123.678);

@everybody
Are people happy with what this is doing??? Things may be PHP version dependent? I am on 5.4.

i.e.
These are ok integers strings: 0123, 000123, -000123. etc
Floats are ok, iff they cast to a whole integer 1.0, 1.0000, etc

@stefan.r
Anyways, I don't want to hold this up any more than necessary, this has been the bane of Postgres users for years.

I think we should put those expectations into a phpunit test - ideally they'd match the current MySQL behaviour (especially for the backport) - did not verify that myself.

Not in this issue but we should also open a follow-up to make things more restrictive in 8.x. https://www.drupal.org/node/00000000001 shouldn't work and the cleanIDs() gives us a chance to fixthat. I had never realised until today that it did.

Is that an issue? intval('000000000000001') === 1. Think backwards compatibility to those systems your granddaddy worked on that used zero padded strings for integers. ;)

Well it means a very high number of valid URLs that all refer to node/1. Unless a site has rel=canonical on that page then it looks like duplicate content, and it also allows caches to be bypassed. It's not very serious (we have other similar behaviour like node/1/ggggggg being valid), but seems worth an issue.

The current mysql behavior is to recognize the following as 1:

1 and '1'
01/001 and '01'/'001'
1.0/1.00 and '1.0'/'1.00'
0x1
0b1

And the following as -1:

-1 and '-1'
-01/-001 and '-01'/'-001'
-1.0/-1.00 and '-1.0'/'-1.00'
-0x1
-0b1

The snippet in #162 and #164 seems to match that behavior in both PHP 5.4 and 5.3, so that is probably what we should go with:

is_numeric($value) && $value == (int) $value;

@catch
No correctly constructed URL would have that format, making it a non-issue imho, as per "node/1/ggg" or "node/0001", unless I'm missing something here. I have seen the former format from errors in contrib, but I have personally never seen zero padded id's in real life before.

However, I guess if you really want to harden the system against any dos attacks it could be worth it, but doing this at the entity load level seems wrong to me, maybe?

@stefan.r
The entity controller negates this with "$ids = array_map('intval', $ids);"; as long as intval() matches the expected behaviour of the strings passing from the menu callbacks via the anonymous function filter. So any issues related to db specific behaviours is a non-issue as all it will see is an real integer. :)

@Alan D. just to clarify, I think the MySQL compatibility thing is not about DB specific issues but more about an entity_load_multiple giving the same result regardless of database used.

Currently in MySQL (without any patch applied), if you pass in array('0001', '7.000') it will load entities 1 and 7. With this patch though, depending on which snippet we use, these IDs will either be filtered out and just one entity would be loaded (or none at all), which would not give any database issues but would still break compatibility, or entities 1 and 7 would still be loaded (which is what we want).

As to whether any of this occurs in real life, padded zeroes might just come up in custom code that deals with remote identifiers (such as 00000123) that are being translated to entity IDs as is. Non-string hexadecimal and binary numbers I find it harder to think of a use case for :)

For people on D7 that don't want to patch core:

I just helped a co-worker out diagnosing an issue where file_entity would try to insert a record with an fid that was the filename. Turns out it was because the patch for this was not on core (I had thought it'd been committed).

Installing @Josh Waihi's sandbox module is still a valid workaround http://drupal.org/sandbox/fiasco/1786222

Downloading and enabling it solved our issues and also solves the /node/"string" errors

Hi,
I encountered the problem on Drupal 7 with module media (I think, see https://www.drupal.org/node/2098335).

I just changed the (Drupal 7) patch from #70 to just check is_null($id).
Aren't all not-null cases OK?
Anyway, this fixed the problem for me.

btw: I think the Drupal7 patches fail just because they are for Drupal 7 and this is a Drupal 8.0.x-dev issue.

By the way your line in cleanIds that does,

return is_int($id) || (is_string($id) && ctype_digit(ltrim($id, '-')) && ($id[0] !== '0' || $id === '0'));

Means you could pass: '-----1' and it will be in the filtered array. Then array_map that applies intval means the id ends up as id 0.

I don't see why ltrim negative sign in the first place. Shouldn't cleanIds be saying we need positive integers.

Rerolled stefan.r's patch in #160.

Thanks a lot @Gisle for the reroll! Setting to needs review to actually been tested...

This adds unit tests as suggested in #165 and changes the integer test to the snippet suggested in #162 and #164:

is_numeric($value) && $value == (int) value;

interdiff

Can you explain why the changes to UserPasswordResetTest was required?

In your valid test cases array you have integer values written in decimal, hex, oct, binary notation. Since they all compile to the same kind of integer it doesn't make sense to test them separately. If one of them works the other will as well. Similarly running the tests for both 1.0 and 1.00 isn't better than just testing one of them. They are both the same value.

@Gisle

The UserPasswordResetTest fix was required because the entity ID sanitization exposed an issue there. The tests were failing as an error was thrown about the one-time login link not being valid anymore after clicking the "Log in" button.

During setUp(), the test fakes the "last login" timestamp to be a time in the past in the database itself, instead of through the actual setChangedTime() method on the user object. This change is not picked up by the tests on time as without the entity ID sanitization the tests will ignore the new, fake "last login" time (and still use the cached, real, "last login" time), so the "last login" times at link generation and when using the one-time login link match and the test passes.

With the ID sanitization however, the route matching in the breadcrumb finds a request to "user/reset", and attempts to load a user entity with the ID "reset". The sanitization empties the array, so when inputting an empty array into buildQuery(), ALL user entities are reloaded (and the last login time gets set to the fake time in the past defined in the test). Later on in the request, the last login time in the login link is compared with the login time in the user object, and as they don't match, the one-time login link is "not valid" anymore and tests fail.

As to the integer tests, that does make sense, I had blindly copied the ones in #164 :)

Actually there was a further issue here, I have edited #187 to reflect this.

When attempting to load an array consisting of only invalid IDs (such as when going to user/reset, where the route matching will recognize this as a valid path and attempt to load a user entity with ID "reset"), the ID sanitization will clear out the whole array and input an empty array into buildQuery. Which will behave the same as NULL and load ALL entities in doLoadMultiple. It will later filter those out in loadMultiple, just under where it says

// Ensure that the returned array is ordered the same as the original
// $ids array if this was passed in and remove any invalid ids.

It's still unnecessary overhead so we'll need to do an extra check and see whether the ID array is empty after the sanitization. If it is empty we return no entities at all instead of returning all entities.

This patch makes sure the array is tested for emptiness after sanitization. It also reverts the fix to the UserPasswordReset patch.

Patch applies.

+++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
@@ -453,6 +459,30 @@ protected function getFromStorage(array $ids = NULL) {
+    if ($id_definition->getType() == 'integer') {
+      $ids = array_filter($ids, function ($id) {
+        return is_numeric($id) && $id == (int) $id;

is not good, check the http://api.drupal.org/api/search/8/_comment_entity_uses_integer_id
Maybe it makes sense to decouple this check.
OTOH is there other way to map field (typed data) properties to specific schema column type

@andypost wasn't this chunk from the patch what you meant to quote instead? Or are the last 2 lines in your chunk (that do the actual integer sanitization) also not good?

+    $definitions = $this->entityManager->getBaseFieldDefinitions($this->entityTypeId);
+    $id_definition = $definitions[$this->entityType->getKey('id')];
+    if ($id_definition->getType() == 'integer') {

Just so I can revise the patch, how is this is different than the code in _comment_entity_uses_integer_id(), other than already knowing the entity type has an ID as we are dealing with ContentEntities?

I've assigned the issue to @Berdir to get his opinion about this change

+++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
@@ -453,6 +459,30 @@ protected function getFromStorage(array $ids = NULL) {
+    if ($id_definition->getType() == 'integer') {
+      $ids = array_filter($ids, function ($id) {
+        return is_numeric($id) && $id == (int) $id;
+      });
+      $ids = array_map('intval', $ids);

This code leads to *hiding the problem* by removing wrong none-integer IDs
IMO better throw some warning at least when none integer ID is removed.

@andypost just to clarify why it silently clears out non-integer entity IDs, this is exactly what MySQL used to do, the point of this patch was to make other DBMSes behave like MySQL.

Yes, no reason to assign this to me, I think applying the same logic as already happen implicitly with MySQL is fine.

On a related note, this issue was causing a number of tests to fail on non-MySQL databases such as PostgreSQL but this may mostly (or entirely) be due to invalid URLs being loaded. Ie. when going to user/password/reset/TOKEN, the breadcrumb builder will attempt to load the non-existing route user/password, causing us to attempt to load a user with ID "password".

If this patch gets committed in its current form, that would become a non-issue, but as @andypost well notes, the current behavior does hide potential issues. So perhaps we can do this same of type checking during entity param conversion as well (in another issue), and worry about exceptions and such there?

Sure, catching the problem in an earlier state would be fine and could show potential problems, but that should be another issue (follow-up) if anyone cares. This is the last major/critical PostgreSQL breaking issue to be fixed and is causing 90% of all failing Drupal 8 PostgreSQL tests. This issue also affects sites running D7 for years, so we need a backport asap.

So, given that a retest will still be green, we should be RTBC.

Let's do it.

For the record, I'm not sure how to port this back to 7.x. There is nothing in 7.x comparable to field definitions. The only way might be to look at the schema and the type there.

Which we will need to profile. The 8.x implementation is not critical for performance, as it is only executed on entity storage cache misses.

#145 was a stab at a 7.x backport.. While the whole database schema is cached in drupal_get_complete_schema(), if upon profiling performance really is degraded and there is no way to remediate that, perhaps we can do the integer sanitization on non-MySQL databases only in 7.x?

Committed/pushed to 8.0.x, thanks!

Moving to 7.x for backport.

Thanks!

Another try at a D7 backport attached, please can someone profile this? It may affect perfomance as per #200

Filed major follow-up #2356943: SqlContentEntityStorage::cleanIds() hides errors to leave this one for d7

@stefan.r: I just applied patch #204 and it went through fine for me. I'm using MSSQL.

Update: Unfortunately I'm still receiving errors like the OP when adding fields to a webform.

Hi i change the order of the calls to array_filter() and array_map (first array_map()) into the method cleanIds() to work with URL's type node/1.json or user/22.xml to first cast the id to integer a later validate it here is the code:

/**
   * Ensures integer entity IDs are valid.
   *
   * 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.
   *
   * @param array $ids
   * The entity IDs to verify.
   * @return array
   * The sanitized list of entity IDs.
   */
  protected function cleanIds($ids) {
    $base_table_schema = drupal_get_schema($this->entityInfo['base table']);
    if (isset($base_table_schema['fields'][$this->idKey]['type'])) {
      $id_type = $base_table_schema['fields'][$this->idKey]['type'];
      if ($id_type == 'serial' || $id_type == 'int') {
        $ids = array_map('intval', $ids);
        $ids = array_filter($ids, function ($id) {
          return is_numeric($id) && $id == (int) $id;
        });
      }
    }
    return $ids;
  }

@metallized: why would you want to hack core to do that, wouldn't this be better dealt with at the webserver level, or in hook_menu in a custom module? With your change, doing node_load('1.json') or going to node/1.json will load a node with ID 1, which you may want to do in your particular case, but in general that should give a 404 error.

@jayhawkfan75: can you post the specific error you were getting? Do you use entitycache (which may require a separate patch)?

@stefan.r: i was having this issue on an installation over a oracle backend but that error has nothing related with the RESTful Web Services module the error was visible when i try to enter to URL's of type <entity_type>/<entity_id>.<format>.

If i try this type of URL's over a MySQL backend without the RESTful Web Services module it works (not a 404 error) redirect me to the entity page (eg: node/1).

Patch #204 worked for me when applied (site is running under PostgreSQL).

...coming from #2047811: Make compatible with non-integer entity IDs

@stefan.r: Sorry. This is the error I receive:
PDOException: SQLSTATE[22018]: [Microsoft][SQL Server Native Client 10.0][SQL Server]Conversion failed when converting the nvarchar value 'components' to data type int.: SELECT revision.[vid] AS [vid], base.[uid] AS [uid], revision.[title] AS [title], revision.[log] AS [log], revision.[status] AS [status], revision.[comment] AS [comment], revision.[promote] AS [promote], revision.[sticky] AS [sticky], revision.[ds_switch] AS [ds_switch], base.[nid] AS [nid], base.[type] AS [type], base.[language] AS [language], base.[created] AS [created], base.[changed] AS [changed], base.[tnid] AS [tnid], base.[translate] AS [translate], revision.[timestamp] AS [revision_timestamp], revision.[uid] AS [revision_uid] FROM {node} base INNER JOIN {node_revision} revision ON revision.nid = base.nid AND revision.vid = :revisionId WHERE ( ([base].[nid] IN (:db_condition_placeholder_0)) ); Array ( [:db_condition_placeholder_0] => 1641 [:revisionId] => components ) in DrupalDefaultEntityController->load() (line 202 of includes\entity.inc).

@jayhawkfan75 that seems like a separate issue unrelated to this patch -- somehow it's trying to load a revision ID called "components", but I doubt that even exists as Webform nodes don't use revisions per #1889390: Make webform nodes support revisions

@stefan.r: Ok, thanks very much for the info and forwarding that link. Sorry for the confusion, and good job on your patch.

The solution is unfortunately insufficient. On my site I stumbled upon this bug (again) with the patch in #204 applied. After quite some debugging I found out that using pathauto in combination with specific URL patterns could throw these fatal errors.

For my blog nodes I used a pattern of blog/[node:title]. This obviously "conflicts" with the Drupal built-in blog/%user_uid_optional-URL, but it still works great except for one thing: editing nodes. When editing a node with that pattern and saving pathauto makes a call to menu_get_item('blog/the-title'). After a while it will end up with a call to user_load('the-title'), which triggers this bug. It seems like menu system ends up with blog/%user_uid_optional being the "perfect" match and therefore tries to load the obviously non-existing user with id the-title. Changing the pathauto pattern to something else (like blogg) works around the problem.

This can easily be reproduced with a Drupal core installation. Either of the following will do:

• drush php-eval "user_load('invalid');"
• Visit /blog/invalid

This only applies to 7.x as blog was removed from Drupal 8 IIRC and the drush command above works just fine without any error (prints nothing as the id is invalid).

@JimmyAx with the patch in 204 I could not reproduce your issue with a simple user_load('invalid'). It doesn't give any errors as the $ids array in DrupalDefaultEntityController::load() still gets emptied in cleanIds() as (ie. line 186 of entity.inc). You're not using another entity controller by any chance?

@stefan.r I tried the above again with a completely clean Drupal core, first without the patch and then with the patch. Without the patch it throws the Exception, but with the patch it works exactly as expected without any errors. It looks like I messed something up yesterday when testing it.

Anyway this patch look fine to me so I'm setting it to RTBC.

Thanks, as per #200 this may still need profiling...

is not compatible with postgresql? caused by this issue? If it is then I will mark that as dupe..

No, that's another issue. This issue is about entity identifiers but that issue is about weights.

Setting to RTBC again.

1. +        $ids = array_filter($ids, function ($id) {
   +          return is_numeric($id) && $id == (int) $id;
   +        });
   

   This will break PHP < 5.3.

2. +    // Ensure integer entity IDs are valid.
   +    if (!empty($ids)) {
   +      $ids = $this->cleanIds($ids);
   +      // If all passed IDs have been filtered out, do not return any entities.
   +      if (empty($ids)) {
   +        return $entities;
   +      }
   

   Is an early return really what we want here? It looks to me like the code further down can still do some things even if $ids is empty.

3. Performance was mentioned as a concern above, but there hasn't been any profiling of this. It looks a little worrisome to me too... I think drupal_get_schema() can be cached but this patch is still calling it for multiple tables on essentially every page request and normally I think that's not a function that's called often.

   Couldn't we at least avoid that by switching the logic around (only loading the schema if there are non-numeric entity IDs in the list)?

   Or even better, we already have a fair amount of schema info about the entity cached in the 'schema_fields_sql' key returned by hook_entity_info() (see drupal_schema_fields_sql())... Couldn't we just get a little extra info when that is being built and cache it there?

4. The Drupal 7 patch is missing a lot of the tests that were in the Drupal 8 patch...

Good points David.

1. Do you know which part of the code would be invalid? (for our purposes in PHP >= 5.2.5 to meet Drupal 7's official requirements). I assume you mean the function as an argument needs to be separated out as a callable function?

2. I think based on what MySQL would have performed this workaround achieves the same end result with PostgreSQL? (i.e. even if conditions or a revision id were set, the buildQuery for a faulty id would have also returned nothing due to this condition:

if ($ids) {
      $query->condition("base.{$this->idKey}", $ids, 'IN');
    }

and the remaining code would pass an empty array in the end anyway after all those downstream checks.

3. This can definitely be improved (at least in theory). I haven't looked into drupal_schema_fields_sql() yet but I assume like you said that there's no need to call drupal_get_schema() when an id is confirmed an integer (since strings are the exception rather than the rule?).

Assuming the PHP < 5.3 incompatibility is rectified and no existing static substitute is found, I suppose you could use something like this instead rather than calling the cleaning function automatically (which can add one extra schema call per page when an integer is passed):

    // Ensure integer entity IDs are valid. 
    if (!empty($ids)) {
			$integer_ids = array_filter($ids, function ($id) {
        return is_numeric($id) && $id == (int) $id;
      });
      if (count($integer_ids) !== count($ids)) {
				$ids = $this->cleanIds($ids);
				// If all passed IDs have been filtered out, do not return any entities.
				if (empty($ids)) {
					return $entities;
				}
			 }
			}

Without profiling it's hard to say if there is any microsecond improvement, if any. And I suppose some memory caching setups will skew/alter the performance impact of various solutions.

4. I haven't looked yet.

I had problem with array_flip() function described here:

https://www.drupal.org/node/2098335

It is somehow connected with media module since warnings appear when media module is enabled.

My problem dissappeared after I have applied #204 patch, but I had to put first part of patch (quoted below) into entity.inc before line 173 in order to valid ids before array_flip() function.

+    // Ensure integer entity IDs are valid.
+    if (!empty($ids)) {
+      $ids = $this->cleanIds($ids);
+      // If all passed IDs have been filtered out, do not return any entities.
+      if (empty($ids)) {
+        return $entities;
+      }

I know very little about coding... and, of course, maybe I did wrong to copy part of patch to wrong place, but my problem dissapeared after I did that... so I am just posting my experience here in case that it is helpful.

Hi devad,

I've never had that "null passed in array" issue in of my installed contrib modules but I've read about it happening.

I don't know with certainty why the code you posted was placed after the cache check but maybe the idea was to avoid unnecessary schema checks in the cleaning function if an id has already been cached.

If that's the case, then I suppose $ids = array_filter($ids, function($id){return !is_null($id);} ); (though function as argument may not work in PHP < 5.3?) could be added above the array_flip(). array_flip() actually performs this automatically but the warning doesn't look pretty on dev environments and if you can't otherwise patch the cause, repeated warnings with error_reporting will add up in performance hits.

Anonymous functions were added in 5.3.0 - http://php.net/manual/en/functions.anonymous.php

Thanks for clarifying.