The Comment blocks test returns 1 fail, 21 exceptions such as:

Undefined index: 1#comment-1 Notice	comment.module	1616	comment_load()

All these exceptions are triggered when using clickLink() on a comment link, such as line 1013 in comment.test

    $this->clickLink($comment1->subject);

The fragment gets encoded as part of the q variables and interpreted as part of the comment id. The fragment should be removed in clickLink() as the webserver cannot deal with it (only the client's browser can).

Note that I'm able to reproduce this on both debian PHP 5.2.6-1+lenny4 and MAMP PHP 5.2.10.

CommentFileSizeAuthor
#13 705854_clickLink_13.patch1.46 KBscor
#11 705854_clickLink_11.patch1.28 KBscor
#8 705854_clickLink_8.patch874 bytesscor
#7 705854_clickLink_7.patch873 bytesscor
#4 705854_clickLink_4.patch1.37 KBscor
#3 705854_clickLink_3.patch1.24 KBscor
#2 705854_clickLink_2.patch1.24 KBscor
#1 705854_clickLink_1.patch664 bytesscor

Comments

scor’s picture

scor commented
Status: Active » Needs review
StatusFileSize
new 705854_clickLink_1.patch664 bytes
scor’s picture

scor commented
StatusFileSize
new 705854_clickLink_2.patch1.24 KB

or more strictly

scor’s picture

scor commented
Title: clickLink() does not work for url_target containing a fragment » clickLink() does not work for url_target containing a fragment and breaks testing
Issue tags: +HEAD broken
StatusFileSize
new 705854_clickLink_3.patch1.24 KB

Just found #671520: Curl and fragment URLs break testing. I suggest to commit the work around present in the attached patch (which disables the fragment before the CURL call), and remove it when the CURL fragment issue has been resolved.

scor’s picture

scor commented
StatusFileSize
new 705854_clickLink_4.patch1.37 KB

making the fragment optional in clickLink().

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Reviewed & tested by the community

I can confirm that this does fix the issue. While it might not be a proper fix, it's a necessary workaround to have the tests pass again.

dries’s picture

dries commented 
+++ modules/simpletest/drupal_web_test_case.php
@@ -1452,6 +1452,10 @@ class DrupalWebTestCase extends DrupalTestCase {
+    // CURL breaks if the url contains a fragment.
+    // @todo remove when http://drupal.org/node/671520 is fixed.
+    unset($options['fragment']);

URL should be written URL instead of url.

+++ modules/simpletest/drupal_web_test_case.php
@@ -1959,7 +1963,13 @@ class DrupalWebTestCase extends DrupalTestCase {
+      $url_target = explode('#',$url_target, 2);

Missing space.

Would be good to have some code comments too.

scor’s picture

scor commented
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new 705854_clickLink_7.patch873 bytes

Here is a more generic approach, which works for any URL with fragment passed to drupalGet(). $path can contain a fragment and url() is meant to handle it fine, so no need to split it in clickLink(). The work CURL around should only live in drupalGet().

scor’s picture

scor commented
StatusFileSize
new 705854_clickLink_8.patch874 bytes

fix typo and space.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Needs review » Fixed

Ick. :\

Committed to HEAD, but let's make sure we get #671520: Curl and fragment URLs break testing fixed so we can remove that hack...

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Fixed » Needs work

Ok, turns out that this triggers other test fails..

There are two tests that use a very special URL that does, beneath other things, also contain an # which needs to be escaped. The above change however strips everything behind the # off. That's not what we want in this case.

I don't see a way to automatically distinguish between these two use cases, any idea?

scor’s picture

scor commented
Status: Needs work » Needs review
StatusFileSize
new 705854_clickLink_11.patch1.28 KB

roll back of #8 and fix of #1 upon chx's advice to isolate the fix in the call rather than evil drupalGet().

chx’s picture

chx commented
Status: Needs review » Reviewed & tested by the community

It's way, way too late to change drupalGet. It's too central and leads to failures :) but fixing clickLink is what this issue is about.

scor’s picture

scor commented
StatusFileSize
new 705854_clickLink_13.patch1.46 KB

Same patch with more comments. This fixes:

Menu router 130 passes, 0 fails, and 0 exceptions
Path alias functionality 102 passes, 0 fails, and 0 exceptions
webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Ok, let's try this one. Committed to HEAD.

Status: Fixed » Closed (fixed)
Issue tags: -HEAD broken

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

Issue tags: +HEAD broken

Restoring issue tags, see #2125755: System messages removed all issue tags during D7 upgrade.