Changing the comment path to the node path triggers a PHP fatal error in l() due to wrongly structured options

In Drupal 7.0 I noticed that Comment URL's were "comment/[cid]#comment-[cid]" (which differed from D6 which uses "node/[nid]#comment-[cid]"). I wanted the old URL back...

Why would you want to do that? This URL is the permalink URL, it simply redirects the user to "node/[nid]#comment-[cid]", taking pagination into account.

subscribe

@nicholasThompson, we implement canonical for these links iirc, so it should be fine for SEO afaik.

However, this looks like a proper strange bug regardless of the use case.

This actually a real bug in the comment.module file.

It defines the $options['attributes']['class'] = 'permilink' where it should be an array of class instead of an array. This cause the l() function to use the [] operator over the 'permalink' string which causes a non catchable PHP fatal error.

Because this URI is built in the template_preprocess_comment() function, it causes a fatal error while rendering is being done, leaving an ugly incomplete rendered page with a PHP stack trace at the bottom.

EDIT: This case happens only when the $_GET['q'] variable (which should be current_path() instead by the way) is the same that the $uri variable in the l() function, which makes it looking like random, but in real life can only happen when the comment path is set to the node path./EDIT

I got the same bug while attempting to build some comments URI as absolute links using the node URI instead (using the fragment) in order to be able to store them in an external application. In my use case comments are stored in a Drupal site but displayed and POSTed using AJAX on another site's node.

Bumping this to critical, because doing a valid alteration we experience PHP fatal errors, and this should never happen, it's not a question of "why would you want to do that" but a question of "this will happen again and again if we don't fix it".

Patch attached.

Also switching back to Drupal 7.x version, because this seems to be a Drupal 7 only bug for now, and probably won't be a Drupal 8.x bug since the comment module is being fully rewritten.

Bumping this to the right component too.

Re-titling.

RTBC.

Change status back to needs review. No "review" postings on this issue yet.

The below line is in the l function in both version 7 and version 8 and assumes any value of key "class" will be an array:

$options['attributes']['class'][] = 'active'

However, to render attributes, drupal_attributes() uses

$data = implode(' ', (array) $data);

which will work just fine if the attribute value is not an array.

In one part of the system, it can be either and array or a not, in another, it must be an array. This inconsistency is not ideal. Are there other parts of the system that assume the value of key "class" will be an array? Should the patch instead go in l() so it works like drupal_attributes()?

Changing priority to major per priority levels of issues.

@mitron Indeed, major seems better than critical. Excellent question, but all over the framework, everywhere, template process functions are awaiting for "class" to always be an array, in most cases.

It might worth the shot to introspect this further, meanwhile, I have sites crashing here, and it seems I am not the only one. I don't want this issue, opened almost 2 years ago by someone elses to stall one year more.

I'm gonna search a bit more.

Core internals:

• authorize.inc, install.inc, locale.inc, menu.inc, pager.inc, tablesort.inc, theme.maintainance.inc; : always uses arrays
• bootstrap.inc (lines 2064, 2065) : documents class should be an array for link options
• common.inc : documents class on elements should be array too
• form.inc : always uses an array for class, except for: radios (theme_radios()) , form_required (theme_form_required_marker()), I consider those two occurences as a bug
• theme.inc : both uses only arrays and document to use arrays

Core modules:

• aggregator, block, blog, book, color, contact, contextual, dblog, field (and submodules), field_ui, file, filter, image, locale, menu, openid, overlay, path, poll, profile, rdf, search, shortcut, simpletest, statistics, taxonomy, tracker, translation, trigger, update, : always uses arrays
• node : always uses arrays except in theme_node_recent_block() function for a theme_table() call
• system : always uses arrays except in theme_status_report()
• toolbar : always uses arrays except one occurence in toolbar_view() which I suspect to become the same bug as this issue in some very rare case (links building)
• user : always uses array except one occurence in theme_user_admin_roles()
• comment (doing this one in the end) : always uses arrays except for the one I fix in the latest patch

My conclusion is we should always use arrays no matter where for the 'class' attribute. Every string use is a bug.

@mitron Most usage of strings are in pure theme_* functions, which make them being safe thanks of the array cast trick in drupal_attributes(). This is also safe for table data cells. Considering this, this reduces the real bug occurence to 2 or 3 potential bugguy cases in the full core. My opinion is that we should only fix those, and not the l() function which is accuratly documented. What do you think?

In general, attributes can be passed in various ways, including strings and arrays. However, the l function currently fails if the class attribute passed to it is not an array. Since in other contexts it is possible to pass a single class as a string, it could create an expectation in a developer that this should be OK with the l function also and the failure is surprising. The l function is thus fragile to breaking due to this expectation. At least one major failure in Drupal 7 resulted from a developer passing a class as a string and not as an array.

The proposed patch, which would need to be backported to version 7, makes the l function more robust. The class can be either an array or a string.

Version change back to version 8 since it would be fixed there first.

First patch is test only and should fail. Second patch is test plus change to the l function.

Please ignore attached patches. See #19.

Correction of wrong test_only patch included in #17.

Please ignore attached patches. See #19.

Third try. Yikes. Notice to self: Work on ONE patch at a time. Use git features to separate multiple in progress patches.

#19: 1200478-17-comment_preprocess_wrong_uri_options.patch queued for re-testing.

#19: 1200478-17-comment_preprocess_wrong_uri_options-test_only.patch queued for re-testing.

@pounard I agree attributes elements should be an array for classes and underlying code that calls l() should be fixed. We should not be adding in checks for if we have an array or not, l() is a critical function as such should be as optimal as possible. And to be honest to me this bug is normal not major or critical.

@marcingy Did you mean to say we should NOT be adding in checks? Do you think that the check will degrade performance?

Changing priority to normal based on opinion of more senior contributor.

@mitron thanks for catching that yes I meant not. Well l() is called many many times on a page on certain pages it can 100 of times. More than that though I am a big fan of defined data structures when ever possible because then it makes things easier to do deal with. The real issue here is that comments should pass an array of classes as l() docs state

http://api.drupal.org/api/drupal/core%21includes%21common.inc/function/l/8

If element 'class' is included, it must be an array

So the patch http://drupal.org/node/1200478#comment-7033406 is correct in my opinion and that patch I would rtbc.

Here is some timing information for the curious:

0.0018 seconds for 1,000 calls to the original line of code

0.2753 seconds for 1,000 calls to the section of code that is new.

The patched lines take more than 150x as long to execute.

In this part of the system the difference is significant.

Change version to 7.x. This bug does not exist in 8.x.

Resubmit patch from #7 written by pounard. Patch file name changed to follow Patch Naming.

Looks like this patch has consensus assuming it passes test.

Whow! Such activity within the night! Ok, read everything what happens, and I still consider fixing the comment template is better for API consistency, and now it seems for micro optimization too. Thanks for rerolling the patch.

RTBC for me too. We should also ensure that other potential buggy cases in core cannot lead to the same error as a followup and this will be a win.

Please, maintainers, commit this.

This code exists in Drupal 8 too, so needs to be committed there first. (For what it's worth, Drupal 8 actualy has two instances of this in the same function, whereas Drupal 7 only has one.)

Quick reroll for Drupal 8, fixing both of them.

Looks good

Indeed, RTBC.

No tests? I mean, it's probably fine since it's fixing essentially a typo, but if we'd had test coverage of permalinks to begin with, this never would've happened.

I'm not even sure a test case would be revelant for this. FYI I'm using the D7 patch on two sites already.

Please commit this at least to 7.x.

#34: comment-preprocess_fix_class-1200478-34.patch queued for re-testing.

I do agree that we still need tests here.
http://drupal.org/core-gates#testing
http://drupal.org/node/767608

It's probably possible to write tests for something like #7 but I think that might fit the category of too much of an edge case to test :) You are basically testing that if someone alters URLs in a particular unusual way and if they do it specifically for comments, that Drupal still works correctly...

I think that runs the risk of writing tests for the symptom rather than the cause, and time would be better spent addressing the cause. To me, it seems like the real cause is that throughout Drupal, $options['attributes']['class'] = 'permalink' and $options['attributes']['class'] = array('permalink') both work pretty much fine (in the sense that Drupal will happily print the class to the page with no errors in both cases).

If that were not the case, this bug would have been caught long ago. I think if you are going to have that kind of flexibility, though, you have to accept that edge case bugs like this will crop up sometimes.

In my opinion, this patch would be fine to commit to Drupal 7 and Drupal 8 as long as there's a followup somewhere to consider making Drupal's attribute handling more strict (e.g. it could expect 'class' to always be an array and refuse to handle it if it isn't). But that's a potentially tricky issue.

I agree with @David_Rothstein here, this wouldn't be a useful test to write, we'd loose more time that we would gain doing that.

I have a small fix to what he said thought: 'class' is documented to be an array pretty much everywhere, so it's actually not a problem due to flexibility, but a real bug that we have here.

I think that runs the risk of writing tests for the symptom rather than the cause, and time would be better spent addressing the cause. To me, it seems like the real cause is that throughout Drupal, $options['attributes']['class'] = 'permalink' and $options['attributes']['class'] = array('permalink') both work pretty much fine (in the sense that Drupal will happily print the class to the page with no errors in both cases).

Yeah, this is what I think needs coverage, not the weird edgecase behavior. I guess I didn't consider that that would be a non-backportable change, though. Let's get a followup issue?

I agree with @David_Rothstein here, this wouldn't be a useful test to write, we'd loose more time that we would gain doing that.

Thanks, but that's not the point. The point is that when we have bugs, we add tests for them to prevent future regressions, regardless of how long it takes. See the link above about the core gates.

I understand you point, but right now I so many sites with so many core patches applied that I really don't care about an obvious typo error fix, I'd just like to see it commited already and remove at least this one from the list I have to maintain. I don't have time to write those tests and already lost too much time here trying to find the bug real source, debugging it, and fix it: my job is done here.

May be a duplicate of #1027936: Comment Preprocess sets a links class as a string, should be an array (which was committed to Drupal 8). Or actually, that issue fixed half of the bug in Drupal 8, but looks like the other half was already fixed somewhere else too.

Found two other places in core where this is wrong. #2177703: Fix link attribute structure

Please make this happen.

34: comment-preprocess_fix_class-1200478-34.patch queued for re-testing.

This somehow needs to be commited at some point?

Looks good

Yep, looks good. I ran into this when testing #2125397: Enable comment_fragment 7.x-1.x-dev to improve issue comment permalinks.

Patch looks good and is correct.

Duly noting: It's a bit weird that it took more than 1 year to get this simple fix committed...

This is fine to commit without test coverage.

The only possible test I could imagine (for D8) would be a functional change to add an explicit condition to the Attribute class that ensures that a 'class' attribute is an array (and throws an exception otherwise). But even that approach could be databable. In any case, separate issue.

51: comment-preprocess_fix_class-1200478-51.patch queued for re-testing.

Committed to 7.x - thanks!

Per #42, #44, and #55 we should have a D8 followup to consider forcing 'class' to always be an array (and that's where any tests would be). I'll create a quick issue for that now.

Created #2259419: Force the 'class' attribute to always be an array, never a string.