Invalid type of $_GET variables causes PHP warnings and notices when treated as strings

Cast $path as a string - $path = trim((string) $path, '/');

Fix looks right, should be simple enough to add tests for this though.

Thanks catch.

Just trying to work out where's best to add the test. Should I add another test module in /modules/simpletest/tests or add a test to one of the existing test modules? If the latter, which test module would you recommend I extend?

First stab at a test. This one should fail, then I'll submit the test together with the fix to see if it passes.

Right, worked out how to run tests locally :-)

Next try at a failing test.

Third time lucky for a failing test.

The test correctly failed, so now adding in the patch to fix the problem.

There's some trailing whitespace in the second hunk, but this looks fine other than that.

Attached patch is a reroll of #9 with the following changes:

• Removed trailing whitespace.
• Removed unneeded reference to this issue.
• Added a comment explaining why we are typecasting as a string.

Oh, and it's got tests, so untagging. :)

Thanks for the tweaks, xjm - still finding my feet on core patches :-)

Shouldn't this be fixed a little farther up in request_path()?

  if (isset($_GET['q'])) {
    // This is a request with a ?q=foo/bar query string. $_GET['q'] is
    // overwritten in drupal_path_initialize(), but request_path() is called
    // very early in the bootstrap process, so the original value is saved in
    // $path and returned in later calls.
    $path = $_GET['q'];
  }

Should maybe have an is_string() check on $_GET['q']. We shouldn't let $_GET['q'] = 'Array' which would happen with the current patch.

@Dave Reid's review is relevant to these related issues as well:
#1242060: Invalid value of page GET variable causes PHP warning
#1242480: Invalid type of $_GET[sort] variable causes PHP warning
#1226480: Invalid search query string causes PHP notice on D7 (warning in D6)

Edit: Basically, we should cast all these values as strings immediately when we assign them from $_GET.

So we thought #15 might resolve all four of these issues, but I realize it doesn't (the other three tests fail). I do think @Dave Reid is correct that we should fix these issues systematically upstream rather than type-checking and typecasting in the specific places that have been found to throw errors.

I'm going to mark the others as duplicates and look into a more complete solution.

For reference, an all-inclusive list of specific $_GET parameters used in core. A lot of these should only ever be strings.

• $_GET['q']
• $_GET['destination']
• $_GET['language'] or custom value
• $_GET['page']
• $_GET['sort']
• $_GET['order']
• $_GET['parent']
• $_GET['token']
• $_GET['weight']
• $_GET['render']
• $_GET['link']
• $_GET['name']
• $_GET['trigger_actions_on_watchdog']
• $_GET['format']
• $_GET['theme']
• $_GET['translation']
• $_GET['target']
• $_GET['pass-reset-token']
• $_GET['name']

Garbage in, garbage out. Unless there's a security issue involved, this won't fix. Fix the calling code instead. Or, as in the case of the OP, enter a proper URL into your browser's address bar.

Yep, I'm ok with that.

I've just watched Crell's Core Conversation update on http://groups.drupal.org/wscci and it looks like this will solve the issue.

This is being resolved in #3162016: [Symfony 6] Retrieving a non-string value from "Symfony\Component\HttpFoundation\InputBag::get()" is deprecated, marking as duplicate.