Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Nothing in request_path() or other functions that affect $_GET['q'] restrict it to 255 characters, and yet that is the maximum length of the path field in statistics_schema().
Similar issue #107824: Convert {watchdog}.referer and {accesslog}.url from VARCHAR to TEXT was solved for the url field by switching from varchar to text.
Comment | File | Size | Author |
---|---|---|---|
#34 | statistics-truncate-1180642-34.patch | 1.62 KB | xjm |
#34 | interdiff-31-34.txt | 1.3 KB | xjm |
#31 | statistics-truncate-tests-only-1180642-31.patch | 1 KB | xjm |
#31 | statistics-truncate-1180642-31.patch | 1.61 KB | xjm |
#28 | 1180642-statistics_schema_path_length_4-rerolled.patch | 1.55 KB | musicnode |
Comments
Comment #1
franzOk, sounds to good to have it fixed both on D7 and D8, and needs upgrade path from D6.
For D8 it should be as simple as:
Comment #2
franzComment #3
marcingy CreditAttribution: marcingy commentedAs per #1284658: MySQL strict mode is causing PDO Exceptions when lengths of varchar are exceeded this should truncate as it is being used in queries eg group by.
Comment #4
franzSo it should go back to varchar(255) and implement a truncate? Or maybe a bigger varchar?
Comment #5
catchedit: last comment was completely wrong.
We require MySQL 5.0.15 - so it looks like a longer varchar is an option here for 7.x and 8.x, not for D6 though.
Comment #6
franzFor D6 we can just backport the truncating part.
So what size would fit for D7+8 ? 512?
I'm attaching a new test that should trigger the error for now. Just left 255 there as a the current limit.
Comment #7
franzComment #9
franzI realized there is no way to set a node alias to be greater than 255. Is there such restrictions to other menu entries?
I've updated the test to use a random path that gets a 404 error but that also trigger the bug, so I've add substr($_GET['q'], 0, 255) prior to inserting on DB.
Comment #10
catchThis looks good but:
The comment is no longer correct since there's no alias here (and it's missing a trailing period as well).
Comment #11
franzThere, fixed.
Giving that this change doesn't affect DB, this doesn't need upgrade path keyword anymore, right?
Comment #12
franzTests are also written too...
Comment #13
catchSorry now there's a grammar error:
Should be 'a path'.
Comment #14
franz:(
Comment #16
franz#14: 1180642-statistics_schema_path_length.patch queued for re-testing.
Comment #17
marcingy CreditAttribution: marcingy commentedLooks good.
Comment #18
marcingy CreditAttribution: marcingy commentedActually :(
Missing a period
Comment #19
franzgrumble...
Comment #21
franz#19: 1180642-statistics_schema_path_length.patch queued for re-testing.
Comment #23
franzHad to re-roll because of some other changes that were committed.
This brings on an issue, as the title field now uses utf8_truncate(), should we also use it with path?
Comment #25
franzHate those statistics tests. I'll spin-off an issue to refactor them into something like assertStatLog(): #1326698: Proper test coverage for Statistics logging
This should work now.
Comment #26
xjmThanks for your work on this patch. It looks good to me. Note that the Drupal 8.x patch will need to be rerolled, because the core directory structure for Drupal 8 has now changed. (For more information, see #22336: Move all core Drupal files under a /core folder to improve usability and upgrades). When the patch has been rerolled, please set the issue back to "Needs Review."
Tagging as novice for the task of rerolling the Drupal 8.x patch.
If you need help rerolling this patch, you can come to core office hours or ask in #drupal-gitsupport on IRC.
Comment #27
musicnode CreditAttribution: musicnode commentedComment #28
musicnode CreditAttribution: musicnode commentedRe-rolled the patch per instructions above. Fingers crossed.
Comment #29
xjmLooks great. Thanks @musicnode!
Comment #30
musicnode CreditAttribution: musicnode commentedYour welcome; Needs review by others, so unassigned
Comment #31
xjmOkay, I noticed a couple things with the comments when re-reading it:
255 needs a unit.
Hm, this isn't quite a sentence. (Actually, my first thought on reading this was "Shaka, when the walls fell.")
Attached cleans up the comments a little. I also clarified the assertion message and removed the
t()
around it (no one is going to translate SimpleTest output).I've also uploaded the test separately to demonstrate that it should fail without the fix.
Comment #32
xjmAlso see the issue summary at #1284658: MySQL strict mode is causing PDO Exceptions when lengths of varchar are exceeded.
Comment #33
xjmJust saw this.
Comment #34
xjmComment #35
Dave ReidLooks good to me. Comments make sense and thanks for providing a test-only patch exposing the failure.
Comment #36
webchickGood bug fix, good test coverage. Thanks!
Committed and pushed to 8.x and 7.x.
Comment #37
franzI see you committed on both D7 and D8, so I'll remove the tag.
Comment #38
xjmActually I think it stays on:
(From http://drupal.org/node/1207020)
The issue is marked fixed, in any case, so no one will try to re-commit the patch. ;)