Right now implementations of hook_popups() can only return an exact path for their popups to take effect. There's also now a handy "*" option that matches all pages. However, we can get more flexibility by using drupal_match_path(), the same function that does path matching for block visibility. This makes it so you can have paths like "node/*" or "admin*", in addition to the exact path matching and the special case "*".
This patch uses drupal_match_path() to add popups. It also removes the "popups_always_scan" variable as I think it conflicts with this approach (since a single page might match several patterns). However, I don't fully understand what popups_always_scan does, so I might be mistaken in removing it.
| Comment | File | Size | Author |
|---|---|---|---|
| #7 | popups_path_matching.patch | 971 bytes | quicksketch |
| #2 | popups_path_matching.patch | 849 bytes | quicksketch |
| popups_path_matching.patch | 1.22 KB | quicksketch |
Comments
Comment #1
starbow commentedCool, glad to learn about the drupal_match_patch function. That is definitely a win.
We still need the popups_always_scan variable though. It is for people who don't want to create a new module, but just want to modify their theme to add a class="popup" here or there.
Comment #2
quicksketchHere's a revised patch that keeps the "popups_always_scan" function. I don't see how it does anything though, since even if it's set to TRUE and there aren't any rules defined for the current path, nothing happens. If it's set to TRUE and there are rules for the current path, it still doesn't matter since those rules would have been executed anyway. So even though I don't know what it does, hopefully this patch is written correctly to use it.
Comment #3
quicksketchAh right I forgot you can just have a class="popups" and it automatically gets a popup. So this makes sense. I think my implementation is correct then with this in mind. Thanks for considering this!
Comment #4
starbow commentedThe code looks fine. As long as it doesn't break anything unexpectedly, I will but it into the next RC...hmm, or maybe I should just release 1.1 and put this into beta1 for 1.2.
Comment #5
starbow commentedHmm, on a deeper look, I am concerned about the speed hit of this change. The look up in popups_init moves from being o(1) to o(n) where n is the number of popup paths to examine, and the coefficent being a pretty hairy regex. And that is being run on every single page render.
I think the power this patch adds is worth having, but I think it might need to be turned on via a checkbox on the popups settings page.
Comment #6
quicksketchI don't think it's devastating in any manner. Drupal already does this for every block it displays on every page.
http://api.drupal.org/api/function/block_list/6
Yes it's definitely a change from O(1) to O(n), but it's a pretty significant increase in functionality. Currently the only way to make popups work on a path like "node/*/edit" is to add the popup to every page load.
Other than that, I don't know what else could be done to put in such a feature.
Comment #7
quicksketchOkay, a slightly better version. Now each path is checked for a "*" with strpos() before running drupal_match_path(). This way we don't run the more expensive drupal_match_path() unless the path is obviously a wildcard match, instead we get away with just a minor strpos().
Comment #8
starbow commentedVery cool. I ran some benchmarks and it looks good.
I think the line
should be:
With that change it tests fine.
I have committed this to the D6 dev branch.
Thanks, this is a significant improvement.
Comment #9
quicksketchThanks Tao!