Working to locate the cause of an error in a separate module, I noticed three notices kept coming up in my error logs: Two "Undefined index: q", and one "Undefined variable: index". All three are in duplicate.module.

function _duplicate_arg() {
  static $arguments, $q;

  if (empty($arguments) || $q != $_GET['q']) {
    $arguments = explode('/', $_GET['q']);
    $q = $_GET['q'];
  }

  if (isset($arguments[$index])) {
    return $arguments[$index];
  }
}

The first two notices would easily be fixed by first checking isset($_GET['q']). I am not sure about the undefined "$index". Should it be "'index'" instead?

I am under a deadline right now, or I would create a patch for the first two notices. I will have time in a couple weeks, if the problem hasn't already been resolved.

CommentFileSizeAuthor
#2 550804_spam_arg_index_v01.patch809 byteskars-t
#1 missing_argument_d5.patch313 bytesliberatr

Comments

liberatr’s picture

liberatr
he/him
Primary language English
Location Portland, OR
commented
Assigned: Unassigned » liberatr
Status: Active » Needs review
StatusFileSize
new missing_argument_d5.patch313 bytes

The comment above the _duplicate_arg() function says it is copied from the source of the arg() function in core.

The core function has the fingerprint:
<?php function arg($index = NULL, $path = NULL) { ... } ?>

However, the function in duplicate.module takes no arguments.

Patch attached. The notice went away for me.

kars-t’s picture

kars-t commented
Version: master » 6.x-1.x-dev
Assigned: liberatr » Unassigned
Priority: Minor » Normal
StatusFileSize
new 550804_spam_arg_index_v01.patch809 bytes

Rerolling the patch againts version 6.x-1.x-dev

gnassar’s picture

gnassar commented

eosrei: could you test that patch? I can see how that would solve the "undefined variable: index" message, but not the "undefined index: q" message.

Otherwise, we're going to have to look at our reuse of the arg() function more carefully. It may not actually be appropriate as implemented.

13rac1’s picture

13rac1 commented

I'll check it as soon as I can. ETA unknown.

AlexisWilke’s picture

AlexisWilke commented

eosrei,

You know, your function does NOT work without the parameter if you do use it when calling the function.

This bug is probably critical for that filter correct/expected functioning...

And it also happens on every page 8-)

Thank you.
Alexis Wilke

gnassar’s picture

gnassar commented
Status: Needs review » Closed (fixed)

The $index parameter is in the function def now. As far as q, I think that's a valid notice. Just means the page doesn't have load arguments (say, a non-node page).

AlexisWilke’s picture

AlexisWilke commented

Note that the patch in #2 was checked in here:
http://drupal.org/cvs?commit=469396

I'm not too sure whether $q will emit a notice, however, gnassar, your comment about notices is wrong. The Drupal Core people clearly say that all variable are expected to be defined or you must use the isset(), empty() functions accordingly.

I also think we don't even need to have that $q variable since it won't change at the time we're using this function.

Thank you.
Alexis

gnassar’s picture

gnassar commented

That's the thing, though, Alexis -- $q *is* expected to be defined. It should exist for all page loads. So, like I said, firing "$q is undefined" is a valid notice.

Why it would've shown as undefined for the OP -- that, I don't know. But $q isn't set by Spam anyway -- it's core.

edit: Figured I'd be more specific -- you can see what $q is from the _arg code itself, when it compares to $_GET['q']. $q is a static for the current page request, which without clean URLs would be http://www.yourdrupalsite.com/?q=page/here (which clean URLs convert to www.yourdrupalsite.com/page/here). So if there's a reason that wouldn't be defined, I can't think of it off the top of my head.

AlexisWilke’s picture

AlexisWilke commented

That's correct. $_GET['q'] is expected to always be defined since it is set by Apache through the .htaccess file and then PHP before your code is executed. So by the time any PHP code is executed, it cannot but be set. (of course, someone could unset($_GET['q']); !?)

  # Rewrite URLs of the form 'x' to the form 'index.php?q=x'.
  RewriteCond %{REQUEST_FILENAME} !-f
  RewriteCond %{REQUEST_FILENAME} !-d
  RewriteCond %{REQUEST_URI} !=/favicon.ico
  RewriteRule ^(.*)$ index.php?q=$1 [L,QSA]

P.S. Ah! Wanted to mention... About $q, it's fine because we check for empty($arguments) and if that is true we do not read $q, and on the next call it's set so it won't generate any notices.