Cannot save text filter config using PostgreSQL if Comment is enabled [blocks installation!]

Aha, this is actually a bug in the text format page.

To reproduce:

a) Install with PostgreSQL and Standard install profile (if only so that the "basic HTML" format is installed).
b) Go to admin/config/content/formats/manage/basic_html (edit the Basic HTML text format).
c) Check the "Display any HTML as plain text" box.
d) Click "Save configuration".

You will get this error (or at least I do):

PDOException: SQLSTATE[22P02]: Invalid text representation: 7 ERROR: invalid input syntax for integer: "basic_html" LINE 4: WHERE (entity_id = 'basic_html') AND (entity_type = 'editor... ^ in PDOStatement->execute() (line 61 of core/lib/Drupal/Core/Database/Statement.php).

Unfortunately it is not giving me the full query here (I checked dblog too and it doesn't have it there).

Looks like this is a filter.module bug, and since I think it means you cannot save any changes to text filters in PostgreSQL, I think it's a "Major".

As a reality check, I verified it works fine in a MySQL installation that is pretty recent. Plus I assume the tests would be failing all over the place if not.

As another reality check, since the error above seemed to say entity_type = 'editor...', I disabled the CKEditor and Text Editor modules. I am now able to save the text filter.

So the problem is apparently in the Text Editor module, some modification it is doing to the text filter configuration form, which is not working in PostgreSQL. Nothing jumped out at me when I looked at the code...

Further debugging information:

a) You have to be editing a text format that already had an editor associated with it.

b) I turned on debugging with backtrace (newly possible today!) and have more information about this error:

PDOStatement->execute([Array])

Drupal\Core\Database\Statement->execute([Array], [Array])

Drupal\Core\Database\Driver\pgsql\Connection->query(SELECT c.cid AS cid
FROM 
{comment} c
WHERE  (entity_id = :db_condition_placeholder_0) AND (entity_type = :db_condition_placeholder_1) , [Array], [Array])

Drupal\Core\Database\Query\Select->execute()

comment_entity_predelete(Drupal\editor\Entity\Editor, editor)

call_user_func_array(comment_entity_predelete, [Array])

Drupal\Core\Extension\ModuleHandler->invokeAll(entity_predelete, [Array])

module_invoke_all(entity_predelete, Drupal\editor\Entity\Editor, editor)

Drupal\Core\Config\Entity\ConfigStorageController->invokeHook(predelete, Drupal\editor\Entity\Editor)

Drupal\Core\Config\Entity\ConfigStorageController->delete([Array])

Drupal\Core\Entity\Entity->delete()

editor_form_filter_admin_format_submit([Array], [Array])

call_user_func_array(editor_form_filter_admin_format_submit, [Array])

Drupal\Core\Form\FormBuilder->executeHandlers(submit, [Array], [Array])

Drupal\Core\Form\FormBuilder->processForm(filter_format_edit_form, [Array], [Array])

Drupal\Core\Form\FormBuilder->buildForm(filter_format_edit_form, [Array])

[followed by a bunch of controller/handle page request stuff that is kind of irrelevant]

Decided we needed an issue summary. :)

I just updated the issue summary with a summary of the actual cause of the problem, and two proposed solutions.

I don't think the latest patch is a real long-term solution to the problem. I think there are really only two possible solutions that will fix this problem permanently:

a) Only allow comments on content entities with integer ID fields, and only check for comments upon entity deletion if the ID is an integer.

b) Allow comments on all content entities, making the entity ID field in the comment table be string-valued instead of integer-valued.

If we do (a), then I think the right check in the pre_delete function would be to do an is_integer() before running the query, in addition to probably (as in the current patch) checking that it is a ContentEntityInterface, and maybe also checking that it is fieldable?

If we do (b) then we need to change the database table, and we should still also probably check that it's a content entity and that it is fieldable.

I also suggested in the updated issue summary that we add a test that is explicitly for this behavior, rather than relying on some code in a Search module test, which could be deleted sometime in the future, to test this.

Isn't this still going to fail for some entities? I don't see why this is the right fix. The current patch is just checking to see that it's a ContentEntity, but there is no guarantee that all ContentEntity types have integer IDs. It may fix it for the text filters, but it doesn't really address the root problem at all.

I had a quick discussion with webchick about this in IRC today.

Just to clarify... I just want to reiterate that the current patch is a "band-aid". It doesn't fix the real root cause of the problem here, which is:
- Comment module, whenever some entity is being deleted, checks to see if it might have comments that need deleting.
- Comment module's database tables have an integer-valued column for entity ID.
- Not all entities have integer IDs; according to #15, not even all Content entities are guaranteed to. (In Core currently, while we have many examples of Config entities that have string IDs, there are currently zero examples (except a test case) of Content entities that have string IDs. However, it is allowed.)
- The database queries that happen when Comment checks to see if there are comments on an entity try to match the entity ID with the integer column. This type of matching of a string on an integer database table column causes exceptions in PostgreSQL. This error is coming up when trying to delete a config entity from the Editor module, which happens whenever you save a text filter config if it previously had an Editor associated with.

The fix in the current patch is to only run this check if the entity is a Content entity. This is OK as long as Content entities all have integer IDs, but will end up failing down the road if a content entity is created with non-integer ID.

So, while the patch does allow the Search module tests to pass, it is not a very good fix for the root cause of the problem. Whether or not to commit this patch it is not my decision to make, but if it were my decision I think I would push for a real fix to be made rather than a temporary solution.

The only real fixes would be:
a) Require all content entities, after all, to have integer IDs.
b) Change comment module to use string IDs in its db table.
c) Only allow comments on entities with integer IDs, and verify this is the case when checking for comments.

I would also be more comfortable with the supplied patch if besides verifying it's a content ID it also made sure the ID it was checking was actually an integer.

I do not see why (c) could not at least be implemented in this patch.

The code that is crashing, after this patch, is:

function comment_entity_predelete(EntityInterface $entity) {
  if ($entity instanceof ContentEntityInterface) {
    $cids = db_select('comment', 'c')
      ->fields('c', array('cid'))
      ->condition('entity_id', $entity->id())

Given that this will *still* fail in PostgreSQL if $entity->id() is anything but an integer, could we not instead do:

function comment_entity_predelete(EntityInterface $entity) {
  if (is_int($entity->id())) {
    $cids = db_select('comment', 'c')
      ->fields('c', array('cid'))
      ->condition('entity_id', $entity->id())

That seems like a simple change that does not add any huge performance problems (maybe it is even more efficient than the existing patch?), and at least gets closer to addressing the real problem, which is that you cannot pass every $entity->id() into this query because {comment}.entity_id is an integer field. I mean, the problem is not that $entity might not be a content entity -- the problem is that the db field is incompatible with non-integers and PostgreSQL is strict about that, right? So why not make the if() statement actually test for the exception-causing issue rather than testing for something that is only by coincidence related?

Agreed on isFieldable() as being a better test that "is it a content entity".

Agreed that the id() might be an integer masquerading as a string. I'm wondering in that case whether we may also need to cast to (int) in the query to avoid the PostgreSQL problems?

Also, we could use is_numeric() to test this, possibly in addition to the "> 0" (I had to run php -a to verify for myself that ('abc' > 0) for sure evaluated to FALSE and ('52' > 0) for sure evaluated to TRUE -- it does work though).

Blocking install with Standard profile in non-English using PostgreSQL brings this up to "critical" status, I think? Updated the issue summary

andypost: The main goal here is to make sure this query does not fail with an exception, and a secondary goal is to not even run the query for entities that do not support comments (hence the isFieldable() check).

The query will fail with an exception if the entity ID is a non-integer.

The patch here is not quite right -- I think it needs to cast ->id() into int after checking is_numeric().

This patch also needs a dedicated test. I do not think we should be relying on a search.module test that could go away at any time to make sure this stays fixed.

OK then. I'm glad to see that we have confirmation via a failing test instead of a hypothethical "it probably works this way".

So, it does look like we should either use > 0 or is_numeric to test for this being an actual integer value, and then I think we also need to cast the value to (int) in the condition() statements in the queries.

I think that we should also put in a code comment, probably with an @todo, that explains why we are even testing this to see if it's numeric in the first place.

The code in the patch looks fine to me, and obviously passes the tests this time.

Could we change the comment from

+  // We support non-numeric IDs but {comment} and {comment_entity_statistics}
+  // tables expect the entity ID to be an integer. We verify that the id is
+  // numeric and cast it to an integer when querying to prevent errors on
+  // Postgres.

to something more like:

Entities can have non-numeric IDs, but {comment} and {comment_entity_statistics} tables have integer columns for entity ID, and PostgreSQL throws exceptions if you attempt query conditions with mismatched types. So, we need to verify that the ID is numeric (even for an entity type that has an integer ID, $entity->id() might be a string containing a number), and then cast it to an integer when querying.

And maybe we should also add:

@todo Should we support non-integer entity IDs for comments?

Thanks! I'm +1 on marking this RTBC now.