The regex replacement strips %blah and replaces it with *. While, the search pattern knows that menu replacement arguments end with a /, the replacement pattern does not. The change is simple, use '[^\/]*' as the replacement string instead of '*'.

I noticed this problem because I added a modal to 'user/%user' and the regex started turning anything user/ into a modal, for example user/logout.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

chx’s picture

FileSize
2.1 KB

Aye but that patch is waaaaaay too little. My patch is PHP 5.3 anyone is welcome to convert down to 5.2.

Dave Reid’s picture

I need comments. Because I'm a dumb maintainer.

catch’s picture

Status: Needs review » Needs work

I'm seeing these when installing an install profile, likely just needs an empty() check for when no paths are modal.

preg_match(): Empty regular expression ctools_automodal.module:53    [warning]
preg_match(): Empty regular expression ctools_automodal.module:53    [warning]
implode(): Invalid arguments passed ctools_automodal.module:42       [warning]
Dave Reid’s picture

@catch Unrelated error or error that happens with either of the patches applied?

catch’s picture

Just the latest patch.

wojtha’s picture

Status: Needs work » Needs review
FileSize
2.42 KB

Trying to fix the bugs reported by catch in #3

jlyon’s picture

This will throw a fatal error on php < 5.3 because of the line:

$convert_to_regexp = function ($value) { return preg_replace('/%[^\/]*/', '[^/]+', $value); };.

Any possible work-arounds? See https://github.com/wojtha/ctools_automodal/issues/1.

Edit: @chx: Obviously you knew that it was php 5.3 because you mentioned it in your post. Sorry for not catching it. I think it is important to make it work in php 5.2 for the issues outlined in the github issue. I'll see if I can figure it out.

jlyon’s picture

This update to #6 makes the patch work with php < 5.3.

chx’s picture

Just FYI, the last bugfix release of 5.2 was two years ago. The last security update was 1.5 years ago. I know, I know. But, still.

acrollet’s picture

re-roll of #8 attached. This makes 2 changes:

1) The not-modal regexp was too greedy when there were no paths in it (@^@)
2) Adds comments per #2

Status: Needs review » Needs work

The last submitted patch, ctools_automodal-regex_too_permissive-1439762-10.patch, failed testing.

acrollet’s picture

Status: Needs work » Needs review
FileSize
3.33 KB

updated patch attached, applies properly against head and eliminates a notice.