PDOException when saving a node on non-English languages

I wonder if you replaced substr() with drupal_substr() would do it, but that doesn't sound right either. Can you provide a way we can reproduce the error?

I have reproduced it with the following code.

dblog_watchdog(array(
  'uid' => 5,
  'type' => 'type',
  'message' => 'a message',
  'variables' => array(),
  'severity' => 1,
  'link' => urldecode('/content/xo%E9%85%B1%E5%87%89%E6%8B%8C%E7%B4%A0%E9%B8%A1%E7%85%A7%E7%83%A7%E9%B8%A1%E9%BB%84%E7%8E%AB%E7%91%B0-%E7%A7%91%E5%B7%9E%E7%9A%84%E5%B0%8F%E4%B9%9D%E5%AF%A8%E6%B2%9F%E7%BB%9D%E7%BE%8E%E9%AB%98%E5%B1%B1%E6%B9%96%E6%B3%8A%E9%85%B1%E5%87%89%E6%8B%8C%E7%B4%A0%E9%B8%A1%E7%85%A7%E7%83%A7%E9%B8%A1%E9%BB%84%E7%8E%AB%E7%91%B0-%E7%A7%91%E5%B7%9E%E7%9A%84%E5%B0%8F%E4%B9%9D%E5%AF%A8%E6%B2%9F%E7%BB%9D%E7%BE%8E%E9%AB%98%E5%B1%B1%E6%B9%96%E6%B3%8A%E9%85%B1%E5%87%89%E6%8B%8C%E7%B4%A0%E9%B8%A1%E7%85%A7%E7%83%A7%E9%B8%A1%E9%BB%84%E7%8E%AB%E7%91%B0-%E7%A7%91%E5%B7%9E%E7%9A%84%E5%B0%8F%E4%B9%9D%E5%AF%A8%E6%B2%9F%E7%BB%9D%E7%BE%8E%E9%AB%98%E5%B1%B1%E6%B9%96%E6%B3%8A%E9%85%B1%E5%87%89%E6%8B%8C%E7%B4%A0%E9%B8%A1%E7%85%A7%E7%83%A7%E9%B8%A1%E9%BB%84%E7%8E%AB%E7%91%B0-%E7%A7%91%E5%B7%9E%E7%9A%84%E5%B0%8F%E4%B9%9D%E5%AF%A8%E6%B2%9F%E7%BB%9D%E7%BE%8E%E9%AB%98%E5%B1%B1%E6%B9%96%E6%B3%8A%E9%85%B1%E5%87%89%E6%8B%8C%E7%B4%A0%E9%B8%A1%E7%85%A7%E7%83%A7%E9%B8%A1%E9%BB%84%E7%8E%AB%E7%91%B0-%E7%A7%91%E5%B7%9E%E7%9A%84%E5%B0%8F%E4%B9%9D%E5%AF%A8%E6%B2%9F%E7%BB%9D%E7%BE%8E%E9%AB%98%E5%B1%B1%E6%B9%96%E6%B3%8A%E9%85%B1%E5%87%89%E6%8B%8C%E7%B4%A0%E9%B8%A1%E7%85%A7%E7%83%A7%E9%B8%A1%E9%BB%84%E7%8E%AB%E7%91%B0-%E7%A7%91%E5%B7%9E%E7%9A%84%E5%B0%8F%E4%B9%9D%E5%AF%A8%E6%B2%9F%E7%BB%9D%E7%BE%8E%E9%AB%98%E5%B1%B1%E6%B9%96%E6%B3%8A%E9%85%B1%E5%87%89%E6%8B%8C%E7%B4%A0%E9%B8%A1%E7%85%A7%E7%83%A7%E9%B8%A1%E9%BB%84%E7%8E%AB%E7%91%B0-%E7%A7%91%E5%B7%9E%E7%9A%84%E5%B0%8F%E4%B9%9D%E5%AF%A8%E6%B2%9F%E7%BB%9D%E7%BE%8E%E9%AB%98%E5%B1%B1%E6%B9%96%E6%B3%8A%E9%85%B1%E5%87%89%E6%8B%8C%E7%B4%A0%E9%B8%A1%E7%85%A7%E7%83%A7%E9%B8%A1%E9%BB%84%E7%8E%AB%E7%91%B0-%E7%A7%91%E5%B7%9E%E7%9A%84%E5%B0%8F%E4%B9%9D%E5%AF%A8%E6%B2%9F%E7%BB%9D%E7%BE%8E%E9%AB%98%E5%B1%B1%E6%B9%96%E6%B3%8A%E9%85%B1%E5%87%89%E6%8B%8C%E7%B4%A0%E9%B8%A1%E7%85%A7%E7%83%A7%E9%B8%A1%E9%BB%84%E7%8E%AB%E7%91%B0-%E7%A7%91%E5%B7%9E%E7%9A%84%E5%B0%8F%E4%B9%9D%E5%AF%A8%E6%B2%9F%E7%BB%9D%E7%BE%8E%E9%AB%98%E5%B1%B1%E6%B9%96%E6%B3%8A%E9%85%B1%E5%87%89%E6%8B%8C%E7%B4%A0%E9%B8%A1%E7%85%A7%E7%83%A7%E9%B8%A1%E9%BB%84%E7%8E%AB%E7%91%B0-%E7%A7%91%E5%B7%9E%E7%9A%84%E5%B0%8F%E4%B9%9D%E5%AF%A8%E6%B2%9F%E7%BB%9D%E7%BE%8E%E9%AB%98%E5%B1%B1%E6%B9%96%E6%B3%8A%E9%85%B1%E5%87%89%E6%8B%8C%E7%B4%A0%E9%B8%A1%E7%85%A7%E7%83%A7%E9%B8%A1%E9%BB%84%E7%8E%AB%E7%91%B0-%E7%A7%91%E5%B7%9E%E7%9A%84%E5%B0%8F%E4%B9%9D%E5%AF%A8%E6%B2%9F%E7%BB%9D%E7%BE%8E%E9%AB%98%E5%B1%B1%E6%B9%96%E6%B3%8A%E9%85%B1%E5%87%89%E6%8B%8C%E7%B4%A0%E9%B8%A1%E7%85%A7%E7%83%A7%E9%B8%A1%E9%BB%84%E7%8E%AB%E7%91%B0-%E7%A7%91%E5%B7%9E%E7%9A%84%E5%B0%8F%E4%B9%9D%E5%AF%A8%E6%B2%9F%E7%BB%9D%E7%BE%8E%E9%AB%98%E5%B1%B1%E6%B9%96%E6%B3%8A%E9%85%B1%E5%87%89%E6%8B%8C%E7%B4%A0%E9%B8%A1%E7%85%A7%E7%83%A7%E9%B8%A1%E9%BB%84%E7%8E%AB%E7%91%B0-%E7%A7%91%E5%B7%9E%E7%9A%84%E5%B0%8F%E4%B9%9D%E5%AF%A8%E6%B2%9F%E7%BB%9D%E7%BE%8E%E9%AB%98%E5%B1%B1%E6%B9%96%E6%B3%8A-lake-isabelle'),
  'request_uri' => 'http://localhost/abc',
  'referer' => 'http://www.google.com',
  'ip' => '255.255.255.255',
  'timestamp' => time(),
));

Attached is my patch which fixes it as described above.

Makes sense, but this needs to be fixed in 8.x first and then backported.

Attempt two.

A few changes after the review with berdir

This is the D7 backport.

Minor bug-fix to the D7 version.

Thanks for the hard work you are doing! I see you are rolling a lot of patches lately. Great! A few minor issues here:

This indicates a failure in the process. <= what process?
You have both is_null($text) vs and $length === NULL to check for NULL-ity and yet the fastest is isset($length).
This comment here: // Requires Unicode module to ensure strings are handled properly is a) not necessary b) not correct since it's not a module. Just delete it :)

Can we have a test please? Amend existing statistics tests for a quick easy win?

This patch addresses chx's review.

I have added to the dblog test code since this calls it directly.

I can't find anything to complain about (OK, the !isset ?: could be switched to isset?: but meh.)

+  if (!function_exists('drupal_substr')) {
+    require_once DRUPAL_ROOT . '/core/includes/unicode.inc';
+  }

If it's require_once()'d elsewhere, why not just require_once() it here?

Hey @catch,

Are you saying that the unicode library should be loaded inside watchdog instead of elsewhere?

Can you clarify your comment please.

@superspring, I think he meant that you can drop the if(), given the include is already being included in several places like that with a simple require_once().

another typo, change 255 to 200.

Reroll with tests only patch.

status change

yup, as expected.

The test no longer applies. Removing the 'needs tests' tag since this has tests now.

Reroll. And a reroll of the test only patch.

Updated patch without the removed drupal_substr function and instead with Unicode::substr. I guess the "test-only" patch from the last post will speak for itself (since a new one would end up identical).

I added a beta phase evaluation to the issue summary, and cleaned it up a bit.

I think this is back to RTBC.

Patch applied cleanly. Re-queuing for testing.

So... Tests still pass. Maybe back to RTBC?

Back to RTBC.

+++ b/core/modules/dblog/src/Logger/DbLog.php
@@ -60,14 +61,14 @@ public function log($level, $message, array $context = array()) {
-        'link' => substr($context['link'], 0, 255),
+        'link' => Unicode::substr($context['link'], 0, 255),

Truncating links seems like a bad idea. Why are we doing this and not changing how dblog stores the field?

It does seem like a bad idea. But I guess that is more of a change than is going on here, though? This issue was created because dblog was trying to truncate to match the length of the db field, but was not successful at it. So the patch is for being successful at truncating and storing messages.

Should we open another issue about storing longer links? In which case this issue would be completely solved by that one :)

The link field type in core specifies storage length of 2048 (we might still need to truncate though to avoid errors when logging errors on really long url pages). Perhaps the fix for 8.0 can simply be to update the watchdog table to match? The fix for 7.x would still need to update the truncation approach.

Hm, I could have sworn I pressed save and uploaded those files?

Trying again...

- Changed the dblog link field to be 2048
- Changed the truncating to match.

As expected :)

Thanks @eiriksm! I think this has addressed @alexpott's concern from ##3. Back to RTBC.

git commit -m 'Issue #1888592 by orb, dzhu: Non latin characters in url, PDOException: dblog_watchdog() in line 155'

+++ b/core/modules/dblog/dblog.install
@@ -53,7 +53,7 @@ function dblog_schema() {
       'link' => array(
         'type' => 'varchar',
-        'length' => 255,
+        'length' => 2048,
         'not null' => FALSE,

+++ b/core/modules/dblog/src/Logger/DbLog.php
@@ -61,14 +62,14 @@ public function log($level, $message, array $context = array()) {
-        'link' => substr($context['link'], 0, 255),
+        'link' => Unicode::substr($context['link'], 0, 2048),

Actually let's make this a text field like the other url columns (referer and location) in this table. And completely remove the necessity to truncate.

Discussed this with @xjm wrt to https://www.drupal.org/node/159605 and have concluded that changing the field type is fine as it is not used in any indexes so this will work on portgres and sqlite.

Given the number of duplicates I've found of this issue - its a major.

Sure let's try this one.

Too bad you beat me to posting comment #46, so the file names are wrong ;)

This will also need to be backported, where I think we don't want to change the schema just for this? So the 7.x will probably be different...

Ah, sorry about that.

I forgot to take out the default value. Now done

Triaging this with @cilefen.

Issue summary is up-to-date and clear.

Simulated the issue using direct calls to the logging API, but need steps to reproduce.

Spoke with alexpott & wimleers, this is definitely a Major issue. Scope is limited to content that may be saved with non-Latin character sets in the path.

Duplicates were noted in the comments but a quick search yielded no results

A Beta Evaluation had already been done and seems correct.

A patch is available but does not apply; needs a re-roll.

Updated the IS with steps to reproduce through clicking and typing (not only programatically).

Here's a reroll of #50.

Ah, you beat me to it.

I was about to upload the files when I got the error message that someone had "changed the Files field".

So I downloaded your patch and it is identical to the file I was going to upload. So more "reviewed" and "tested" than that it can hardly become I think. :)

I'll leave the status so someone else can look at it as well (and so the testbot can finish).

Ah sorry. The files got included anyway. Oh well, they are identical at least (and it includes another test-only patch).

...test only failed as expected.

I think this is at RTBC based on #51.

Committed/pushed to 8.0.x, thanks!

Moving to 7.x for backport. This also needs a head2head issue (although uninstalling then re-installing dblog would cover it really).

(Saving proposed issue credit for discussion and triage participants at LA. We missed them from the 8.0.x commit message unfortunately for this issue, but we can ensure they get credit stored on d.o.)

Here is a d7 patch

Sorry. Forgot to put it to needs review.

OK, so here is a patch that is a little less "disruptive". Keeping in mind that @Berdir in #48 mentioned we should probably not change the schema in d7.

At least the test-only patch proved the point :)

Hm. Probably should have read the actual error messages for the failures.

Here is one that should pass. The only thing left to decide is if we want to alter the schema or not? (this patch does not alter the schema).

I was wrong.

Patch #68 solves my problem. Please commit it.

Committed to 7.x - thanks!

Pretty sure we could change the database schema in Drupal 7 if we thought it would be helpful to (I seem to recall making an exactly analogous varchar=>text change to some other database table not so long ago). However the patch here certainly seems sufficient to fix the bug.

#2671344: Better documentation for suspicious encoded string in dblog tests

New install Drupal 7.44 which appears to have this patch in it
PDOException: in dblog_watchdog() (line 163 when creating new node
believe issue may be 8bit quote character instead of straight apostrophe in node title?

@csc4 can you create a new issue with details and link to it here?