Problem/Motivation
I got the following error as I try to save a node when non-English language (Chinese in my case) is enabled. The problem does not exist when English is the default language.
PDOException: 在 dblog_watchdog() (行 157 在 D:\xampp\htdocs\modules\dblog\dblog.module).
When I catch the exception, I found the following issue:
Incorrect string value: '\xE6\x9F' for column 'link' at row 1
The problematic code is:
Database::getConnection('default', 'default')->insert('watchdog')
->fields(array(
'uid' => $user_uid,
'type' => substr($log_entry['type'], 0, 64),
'message' => $log_entry['message'],
'variables' => serialize($log_entry['variables']),
'severity' => $log_entry['severity'],
'link' => substr($log_entry['link'], 0, 255), // <----- problematic
'location' => $log_entry['request_uri'],
'referer' => $log_entry['referer'],
'hostname' => substr($log_entry['ip'], 0, 128),
'timestamp' => $log_entry['timestamp'],
))
->execute();
In my case, the link is: 查看
So, substr($log_entry['link'], 0, 255) truncates in the middle of the Chinese characters: 查看.
I tried to reduce the substr length to 200: substr($log_entry['link'], 0, 200), then I don't see the problem.
I don't care about such logs, I'd be happy if it is just caught. But I'm sure there is a nice way to fix it.
To reproduce the issue one can for example store a node with a long URL alias including non-standard characters. You can use some of the Chinese characters above or just this exact URL alias:
åååååååååååååååååååååååååååååååååååååååååååååååååååååååååååååååååååååååååååååååååååååååååååååååååååååååååååååååååååååååååååååååååååååååååååååååååååååååååååååååååååååååååååååååååååååååååååååååååååååååååååååååååååååååååååå
Proposed resolution
See patch.
Remaining tasks
User interface changes
API changes
Beta phase evaluation
Issue category | Bug because PDOExceptions |
---|---|
Prioritized changes | The main goal of this issue is to fix a bug and reduce fragility of code. The bug also exists in Drupal 7. |
Comment | File | Size | Author |
---|---|---|---|
#68 | pdoexception_when-1328014-68.patch | 5.03 KB | eiriksm |
#66 | pdoexception_when-1328014-66.patch | 4.81 KB | eiriksm |
#62 | pdoexception_when-1328014-62-test-only.patch | 3.82 KB | eiriksm |
#62 | pdoexception_when-1328014-62.patch | 5.76 KB | eiriksm |
#54 | pdoexception_when-1328014-53-test-only.patch | 3.71 KB | eiriksm |
Comments
Comment #0.0
sf_wind CreditAttribution: sf_wind commentedI want to show the raw link content instead of show the actual link
Comment #0.1
sf_wind CreditAttribution: sf_wind commentedOK, my previous try was not successful
Comment #1
franzI 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?
Comment #2
superspring CreditAttribution: superspring commentedI have reproduced it with the following code.
Attached is my patch which fixes it as described above.
Comment #3
superspring CreditAttribution: superspring commentedComment #4
BerdirMakes sense, but this needs to be fixed in 8.x first and then backported.
Comment #6
superspring CreditAttribution: superspring commentedAttempt two.
Comment #7
superspring CreditAttribution: superspring commentedA few changes after the review with berdir
Comment #8
superspring CreditAttribution: superspring commentedThis is the D7 backport.
Comment #9
superspring CreditAttribution: superspring commentedMinor bug-fix to the D7 version.
Comment #10
chx CreditAttribution: chx commentedThanks 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?
Comment #11
superspring CreditAttribution: superspring commentedThis patch addresses chx's review.
I have added to the dblog test code since this calls it directly.
Comment #12
chx CreditAttribution: chx commentedI can't find anything to complain about (OK, the !isset ?: could be switched to isset?: but meh.)
Comment #13
catchIf it's require_once()'d elsewhere, why not just require_once() it here?
Comment #14
superspring CreditAttribution: superspring commentedHey @catch,
Are you saying that the unicode library should be loaded inside watchdog instead of elsewhere?
Can you clarify your comment please.
Comment #15
franz@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().
Comment #15.0
franzanother typo, change 255 to 200.
Comment #18
eiriksmReroll with tests only patch.
Comment #19
eiriksmstatus change
Comment #21
eiriksmyup, as expected.
Comment #22
jhedstromThe test no longer applies. Removing the 'needs tests' tag since this has tests now.
Comment #23
eiriksmReroll. And a reroll of the test only patch.
Comment #26
eiriksmUpdated 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).
Comment #27
jhedstromI added a beta phase evaluation to the issue summary, and cleaned it up a bit.
I think this is back to RTBC.
Comment #28
jhedstromComment #31
piyuesh23 CreditAttribution: piyuesh23 commentedComment #32
piyuesh23 CreditAttribution: piyuesh23 at QED42 commentedPatch applied cleanly. Re-queuing for testing.
Comment #34
eiriksmSo... Tests still pass. Maybe back to RTBC?
Comment #36
jhedstromBack to RTBC.
Comment #37
alexpottTruncating links seems like a bad idea. Why are we doing this and not changing how dblog stores the field?
Comment #38
eiriksmIt 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 :)
Comment #39
jhedstromThe 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.
Comment #40
eiriksmHm, 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.
Comment #42
eiriksmAs expected :)
Comment #43
jhedstromThanks @eiriksm! I think this has addressed @alexpott's concern from ##3. Back to RTBC.
Comment #44
alexpottgit commit -m 'Issue #1888592 by orb, dzhu: Non latin characters in url, PDOException: dblog_watchdog() in line 155'
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.
Comment #45
alexpottDiscussed 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.
Comment #46
alexpottGiven the number of duplicates I've found of this issue - its a major.
Comment #47
eiriksmSure let's try this one.
Too bad you beat me to posting comment #46, so the file names are wrong ;)
Comment #48
BerdirThis 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...
Comment #50
eiriksmAh, sorry about that.
I forgot to take out the default value. Now done
Comment #51
phillamb168 CreditAttribution: phillamb168 as a volunteer commentedTriaging 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.
Comment #52
eiriksmUpdated the IS with steps to reproduce through clicking and typing (not only programatically).
Comment #53
jhedstromHere's a reroll of #50.
Comment #54
eiriksmAh, 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).
Comment #55
eiriksmAh sorry. The files got included anyway. Oh well, they are identical at least (and it includes another test-only patch).
Comment #57
eiriksm...test only failed as expected.
Comment #58
jhedstromI think this is at RTBC based on #51.
Comment #60
catchCommitted/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).
Comment #61
xjm(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.)
Comment #62
eiriksmHere is a d7 patch
Comment #63
eiriksmSorry. Forgot to put it to needs review.
Comment #66
eiriksmOK, 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 :)
Comment #68
eiriksmHm. 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).
Comment #69
eiriksmComment #70
cilefen CreditAttribution: cilefen commentedComment #71
cilefen CreditAttribution: cilefen commentedI was wrong.
Comment #72
DuaelFrComment #73
eugene.ilyin CreditAttribution: eugene.ilyin as a volunteer and at DrupalJedi commentedPatch #68 solves my problem. Please commit it.
Comment #75
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedCommitted 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.
Comment #78
cilefen CreditAttribution: cilefen commented#2671344: Better documentation for suspicious encoded string in dblog tests
Comment #79
csc4 CreditAttribution: csc4 commentedNew install Drupal 7.44 which appears to have this patch in it
PDOException: in dblog_watchdog() (line 163
when creating new nodebelieve issue may be 8bit quote character instead of straight apostrophe in node title?
Comment #80
csc4 CreditAttribution: csc4 commentedComment #81
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commented@csc4 can you create a new issue with details and link to it here?
Comment #86
Eric_A CreditAttribution: Eric_A commented