Closed (won't fix)
Project:
Drupal core
Version:
6.x-dev
Component:
base system
Priority:
Normal
Category:
Bug report
Assigned:
Unassigned
Reporter:
Created:
6 Feb 2008 at 15:15 UTC
Updated:
6 Feb 2008 at 16:06 UTC
If you pass a non-array to the options argument of l(), it can potentially blow up with undefined index errors. I found it to blow up quite badly in the menu list using a custom template -- to the point that it was halting all other execution.
It was my mistake for not calling l() correctly, of course, but the point still stands. While it is good coding practice to pass the correct arguments, an optional argument should be checked for validity before use. It is not required, and it's a jungle out there.
The unsuspecting end-user without a lot of dev skills may probably never figure out that a module they installed is bombing their site because it uses an incorrectly typed optional argument to l(). And if it breaks their site in a serious way, they probably will have pretty serious second thoughts about Drupal itself.
| Comment | File | Size | Author |
|---|---|---|---|
| safer_l.diff | 792 bytes | davideads |
Comments
Comment #1
t-dub commentedIt looks like execution halts if the problem makes it all the way to the menu cache without being detected. That seems pretty critical. The patch catches it early enough though.
It seems like there might be other instances of this sort of thing lurking in odd places (where inadvertently supplying a NULL argument for something optional creates unanticipated problems).
Comment #2
gábor hojtsyWell, l() has a well defined API, and instead of putting in safety nets everywhere, we prefer to educate people, and fail when they write sloppy code.
Comment #3
davideads commentedHowever, people do write sloppy code. I don't think the well defined API argument holds because this isn't a required part of the API. Note that I didn't add any checks on required arguments -- that's because they are required. But options is not required, and I think it perfectly reasonable to call it like so:
If my options are simply a
nullvalue -- which seems to me like a reasonable value for options --l()breaks. How is that not a problem?I agree that for purity's sake, this isn't the best. But still, shouldn't Drupal be defensive in its handling of optional arguments in core functions? Even the best of us make mistakes, both honest and sloppy. If such a mistake can make Drupal completely blow up in certain cases, shouldn't it be accounted for?
Comment #4
davideads commentedMy apologies -- I didn't mean to change the status. I did, however, want to make my argument part of the public record :)
Comment #5
davideads commentedMy apologies -- I didn't mean to change the status. I did, however, want to make my argument part of the public record :)