Support from Acquia helps fund testing for Drupal Acquia logo

Comments

chertzog’s picture

Status: Active » Needs review
FileSize
588 bytes

Patch attached.

chertzog’s picture

Assigned: Unassigned » chertzog
kim.pepper’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me. Thanks chertzog!

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Needs a reroll... and we already have the request object so this patch is even simpler...

curl https://drupal.org/files/1999442_1-remove-GET-variable-toolbar.patch | git a
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100   588  100   588    0     0    501      0  0:00:01  0:00:01 --:--:--   934
error: patch failed: core/modules/toolbar/toolbar.module:130
error: core/modules/toolbar/toolbar.module: patch does not apply
kim.pepper’s picture

Status: Needs work » Needs review
FileSize
627 bytes

Uses existing request object as per #4.

Crell’s picture

I don't know what this code is doing, but modifying the request is the correct action in about 0.00053% of cases. And those are Drupal design flaws we haven't fixed yet. The odds of this being one of them are low. :-) What's going on in this module, and can we change it to not be wrong?

Crell’s picture

Bumping. I'm especially confused by the use of $_GET['q'], since that's long since been eliminated as a path value.

ParisLiakos’s picture

this is either cruft and can be safely removed, or it hides a bug..$_GET['q'] is definitely not working..i guess some input from a maintainer of toolbar module is needed

kim.pepper’s picture

From MAINTAINERS.txt:

Toolbar module
- ?
kim.pepper’s picture

#5: 1999442-toolbar-request-5.patch queued for re-testing.

Crell’s picture

Oh good.

Let's try removing it entirely and see what testbot says breaks. :-)

kim.pepper’s picture

Removed reference to $_GET as per #11. Seeing what the bot thinks.

Crell’s picture

Status: Needs review » Reviewed & tested by the community

OK then! :-)

YesCT’s picture

Issue tags: -Needs reroll +RTBC July 1

This issue was RTBC and passing tests on July 1, the beginning of API freeze.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

I've manually tested the toolbar with page caching enabled and everything is still working as expected.

Committed 7617975 and pushed to 8.x. Thanks!

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