Hi,

I have some code which produces a popup window with the url .../?q=pp_ed/insert/ifimage/start?M where ?M is the css_js_query_string. It usually works fine, except in the following case ...

If css_js_query_string is 'q'. the url becomes .../?q=pp_ed/insert/ifimage/start&q. This breaks, as the second 'q' makes Drupal display the frontpage.

For example, try the following on a Drupal website ... .../?q=node/add/story?q or .../?q=admin?q

I would suggest that 'q' should not be a possible character generated by _drupal_flush_css_js() for css_js_query_string.

My suggested patch is to change $characters in _drupal_flush_css_js() from

$characters = 'abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789';

to

$characters = 'abcdefghijklmnoprstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789';

(taking out the 'q').

It's a trivial change, so I didn't generate a patch file for this. However, I can generate a patch file if requested (just tell me which version to patch against).

Also, I tested this with Drupal 6.4, but the relevant 6.x CVS code hasn't changed so it still applies.

Thank you.

CommentFileSizeAuthor
#7 320206-q_in_css_js_query.diff791 bytesjeffschuler
#3 node-320206-6.5.patch769 bytesbengtan
#2 node-320206-7.x.patch803 bytesbengtan

Comments

Anonymous’s picture

Anonymous (not verified) commented
Status: Needs review » Active

No patch file then there is nothing to review.

EDIT: See http://drupal.org/patch

bengtan’s picture

bengtan commented
Version: 6.x-dev » 7.x-dev
StatusFileSize
new node-320206-7.x.patch803 bytes

Patch for 7.x (October 13 2008) snapshot attached.

bengtan’s picture

bengtan commented
Status: Active » Needs review
StatusFileSize
new node-320206-6.5.patch769 bytes

Patch for 6.5 attached (which is the current latest DRUPAL-6 at time of posting).

bengtan’s picture

bengtan commented
Version: 7.x-dev » 6.x-dev

Bump.

bengtan’s picture

bengtan commented
Version: 6.x-dev » 7.x-dev

Bump (in the hope someone will take this up).

Status: Needs review » Needs work

The last submitted patch failed testing.

jeffschuler’s picture

jeffschuler
Location Boulder, Colorado
commented
Status: Needs work » Needs review
StatusFileSize
new 320206-q_in_css_js_query.diff791 bytes

Careful to patch from Drupal root rather than your workspace root.

bengtan’s picture

bengtan commented

Thanks for that. Now hopefully a 7.x maintainer will look at this soon.

conchur’s picture

conchur commented

This is still an issue in Drupal 6.10, and breaks the image assist (drupalimage) plugin for TinyMCE.

When 'q' is used as the css_js_query_string it causes a broken img_assist popup (iframe src url="/index.php?q=img_assist/load/tinymce&textarea=edit-body&q") when using TinyMCE and the WYSIWIG module - popup simply shows the front-page instead of the expected image browser. Obviously this doesn't afect the other popups since they're actual static files, just the image assist browser which is mapped to a view.

Clearing the caches fixes it in the short-term, and patching common.inc as above prevents it happening again!

bengtan’s picture

bengtan commented

Ah, good, someone else can independently duplicate the issue. Hopefully that puts a bit more urgency to it.

@conchur: I think the Drupal core patch policy is that all bugfixes go to the 7.x branch first and then get backported to 6.x. Hence I've switched this to the 7.x issue.

Status: Needs review » Needs work

The last submitted patch failed testing.

burningdog’s picture

burningdog commented

The patch works against this specific use-case, but breaks all sorts of other things (like other javascript functionality, and simpletest). Looks like a different solution is needed, rather than simply removing "q" from $characters in _drupal_flush_css_js() - and no, I don't have any ideas :(

bengtan’s picture

bengtan commented

@12: Thanks for that. Not being familiar with the process of auto-testing for core, I didn't realise this.

I do wish, though, that someone could have notified me earlier so I could do something about it (but that is a gripe for another day).

Do you know how I can get information on the specific failed tests?

Or are developers supposed to set up their own 7.x system to duplicate them?

eric_a’s picture

eric_a commented
Status: Needs work » Closed (duplicate)

The very recent #454992: JavaScript query string breaks $_GET['q'] is a duplicate of #320206: _drupal_flush_css_js: css_js_query_string should not be 'q', but Sun's patches actually made the test bots happy while Bengtan's for some reason did not.
We need this issue to be resolved as quickly as possible. Bengtan, in the interest of this I'm proposing we all join forces in #454992.

bengtan’s picture

bengtan commented

Alright, I'm going to shift my attention to the other thread. Nobody else please comment any further on this one. Thank you.