Support from Acquia helps fund testing for Drupal Acquia logo

Comments

franz’s picture

Status: Active » Needs review
FileSize
553 bytes

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:

franz’s picture

marcingy’s picture

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.

franz’s picture

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

catch’s picture

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.

franz’s picture

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.

franz’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

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

franz’s picture

Status: Needs work » Needs review
FileSize
1.44 KB

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.

catch’s picture

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

franz’s picture

Status: Needs work » Needs review
Issue tags: -Upgrade path
FileSize
1.44 KB

There, fixed.

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

franz’s picture

Issue tags: -Needs tests

Tests are also written too...

catch’s picture

Status: Needs review » Needs work

Sorry now there's a grammar error:

+    // Create an path longer than 255.

Should be 'a path'.

franz’s picture

Status: Needs work » Needs review
FileSize
1.43 KB

:(

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

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

franz’s picture

Status: Needs work » Needs review
Issue tags: +Needs backport to D7
marcingy’s picture

Status: Needs review » Reviewed & tested by the community

Looks good.

marcingy’s picture

Status: Reviewed & tested by the community » Needs work

Actually :(

// Test logging the big path truncated

Missing a period

franz’s picture

Status: Needs work » Needs review
FileSize
1.44 KB

grumble...

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

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

franz’s picture

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.

franz’s picture

Status: Needs work » Needs review
FileSize
1.51 KB

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.

franz’s picture

Status: Needs work » Needs review
FileSize
1.51 KB

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.

xjm’s picture

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.

musicnode’s picture

Assigned: Unassigned » musicnode
musicnode’s picture

Status: Needs work » Needs review
FileSize
1.55 KB

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

xjm’s picture

Issue tags: -Novice

Looks great. Thanks @musicnode!

musicnode’s picture

Assigned: musicnode » Unassigned

Your welcome; Needs review by others, so unassigned

xjm’s picture

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.

xjm’s picture

xjm’s picture

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

Just saw this.

xjm’s picture

Dave Reid’s picture

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.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Good bug fix, good test coverage. Thanks!

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

franz’s picture

Issue tags: -Needs backport to D7

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

xjm’s picture

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.