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.

Files: 
CommentFileSizeAuthor
#34 statistics-truncate-1180642-34.patch1.62 KBxjm
PASSED: [[SimpleTest]]: [MySQL] 33,989 pass(es).
[ View ]
#34 interdiff-31-34.txt1.3 KBxjm
#31 statistics-truncate-tests-only-1180642-31.patch1 KBxjm
FAILED: [[SimpleTest]]: [MySQL] 33,974 pass(es), 2 fail(s), and 2 exception(es).
[ View ]
#31 statistics-truncate-1180642-31.patch1.61 KBxjm
PASSED: [[SimpleTest]]: [MySQL] 33,971 pass(es).
[ View ]
#28 1180642-statistics_schema_path_length_4-rerolled.patch1.55 KBmusicnode
PASSED: [[SimpleTest]]: [MySQL] 33,761 pass(es).
[ View ]
#25 1180642-statistics_schema_path_length.patch1.51 KBfranz
PASSED: [[SimpleTest]]: [MySQL] 33,768 pass(es).
[ View ]
#23 1180642-statistics_schema_path_length.patch1.51 KBfranz
FAILED: [[SimpleTest]]: [MySQL] 33,770 pass(es), 2 fail(s), and 0 exception(es).
[ View ]
#19 1180642-statistics_schema_path_length.patch1.44 KBfranz
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1180642-statistics_schema_path_length_2.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]
#14 1180642-statistics_schema_path_length.patch1.43 KBfranz
PASSED: [[SimpleTest]]: [MySQL] 33,754 pass(es).
[ View ]
#11 1180642-statistics_schema_path_length.patch1.44 KBfranz
PASSED: [[SimpleTest]]: [MySQL] 33,651 pass(es).
[ View ]
#9 1180642-statistics_schema_path_length.patch1.44 KBfranz
PASSED: [[SimpleTest]]: [MySQL] 33,274 pass(es).
[ View ]
#6 statistics-large-path-test.patch1.13 KBfranz
FAILED: [[SimpleTest]]: [MySQL] 33,271 pass(es), 0 fail(s), and 1 exception(es).
[ View ]
#1 1180642-statistics_schema_path_length.patch553 bytesfranz
PASSED: [[SimpleTest]]: [MySQL] 33,650 pass(es).
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new553 bytes
PASSED: [[SimpleTest]]: [MySQL] 33,650 pass(es).
[ View ]

Ok, 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:

Status:Needs review» Needs work

As 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.

So it should go back to varchar(255) and implement a truncate? Or maybe a bigger varchar?

Priority:Normal» Major

edit: 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.

StatusFileSize
new1.13 KB
FAILED: [[SimpleTest]]: [MySQL] 33,271 pass(es), 0 fail(s), and 1 exception(es).
[ View ]

For 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.

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, statistics-large-path-test.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.44 KB
PASSED: [[SimpleTest]]: [MySQL] 33,274 pass(es).
[ View ]

I 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.

Status:Needs review» Needs work

This looks good but:

+    // Create an alias longer than 255
+    $big_path = $this->randomString(256);

The comment is no longer correct since there's no alias here (and it's missing a trailing period as well).

Status:Needs work» Needs review
Issue tags:-upgrade path
StatusFileSize
new1.44 KB
PASSED: [[SimpleTest]]: [MySQL] 33,651 pass(es).
[ View ]

There, fixed.

Giving that this change doesn't affect DB, this doesn't need upgrade path keyword anymore, right?

Issue tags:-Needs tests

Tests are also written too...

Status:Needs review» Needs work

Sorry now there's a grammar error:

+    // Create an path longer than 255.

Should be 'a path'.

Status:Needs work» Needs review
StatusFileSize
new1.43 KB
PASSED: [[SimpleTest]]: [MySQL] 33,754 pass(es).
[ View ]

:(

Status:Needs review» Needs work
Issue tags:-needs backport to D7

The last submitted patch, 1180642-statistics_schema_path_length.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+needs backport to D7

Status:Needs review» Reviewed & tested by the community

Looks good.

Status:Reviewed & tested by the community» Needs work

Actually :(

// Test logging the big path truncated

Missing a period

Status:Needs work» Needs review
StatusFileSize
new1.44 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1180642-statistics_schema_path_length_2.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]

grumble...

Status:Needs review» Needs work
Issue tags:-needs backport to D7

The last submitted patch, 1180642-statistics_schema_path_length.patch, failed testing.

Status:Needs work» Needs review

Status:Needs review» Needs work
Issue tags:+needs backport to D7

The last submitted patch, 1180642-statistics_schema_path_length.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.51 KB
FAILED: [[SimpleTest]]: [MySQL] 33,770 pass(es), 2 fail(s), and 0 exception(es).
[ View ]

Had 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?

Status:Needs review» Needs work

The last submitted patch, 1180642-statistics_schema_path_length.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.51 KB
PASSED: [[SimpleTest]]: [MySQL] 33,768 pass(es).
[ View ]

Hate 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.

Status:Needs review» Needs work
Issue tags:+Novice

Thanks 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.

Assigned:Unassigned» musicnode

Status:Needs work» Needs review
StatusFileSize
new1.55 KB
PASSED: [[SimpleTest]]: [MySQL] 33,761 pass(es).
[ View ]

Re-rolled the patch per instructions above. Fingers crossed.

Issue tags:-Novice

Looks great. Thanks @musicnode!

Assigned:musicnode» Unassigned

Your welcome; Needs review by others, so unassigned

StatusFileSize
new1.61 KB
PASSED: [[SimpleTest]]: [MySQL] 33,971 pass(es).
[ View ]
new1 KB
FAILED: [[SimpleTest]]: [MySQL] 33,974 pass(es), 2 fail(s), and 2 exception(es).
[ View ]

Okay, I noticed a couple things with the comments when re-reading it:

+++ b/core/modules/statistics/statistics.testundefined
@@ -128,6 +128,16 @@ class StatisticsLoggingTestCase extends DrupalWebTestCase {
+    // Create a path longer than 255.

255 needs a unit.

+++ b/core/modules/statistics/statistics.testundefined
@@ -128,6 +128,16 @@ class StatisticsLoggingTestCase extends DrupalWebTestCase {
+    // Test logging the big path truncated.

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.

This brings on an issue, as the title field now uses utf8_truncate(), should we also use it with path?

Just saw this.

StatusFileSize
new1.3 KB
new1.62 KB
PASSED: [[SimpleTest]]: [MySQL] 33,989 pass(es).
[ View ]

Status:Needs review» Reviewed & tested by the community

Looks good to me. Comments make sense and thanks for providing a test-only patch exposing the failure.

Status:Reviewed & tested by the community» Fixed

Good bug fix, good test coverage. Thanks!

Committed and pushed to 8.x and 7.x.

Issue tags:-needs backport to D7

I see you committed on both D7 and D8, so I'll remove the tag.

Issue tags:+needs backport to D7

Actually I think it stays on:

Note: This tag should generally remain even after the backport has been written, approved, and committed.

(From http://drupal.org/node/1207020)

The issue is marked fixed, in any case, so no one will try to re-commit the patch. ;)

Automatically closed -- issue fixed for 2 weeks with no activity.