Posted by yakoub on July 15, 2011 at 6:32pm
7 followers
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | overlay.module |
| Category: | feature request |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs work |
Issue Summary
replace path_get_admin_paths with path_get_tagged_paths
this way a module can turn overlay for certain paths without giving it the admin property as well
currently to turn overlay on, hook_admin_paths must be implemented
which will give the 'admin' property to the path as well, although it was not intended .
Comments
#1
attached patch defines new hook : hook_TAG_paths()
so that overlay now may call path_get_tagged_paths('overlay')
and a module requiring the overlay will implement hook_overlay_paths()
#2
got the status wrong in first comment
#3
Hm, interesting patch! It could perhaps be useful to generalize the path_get_admin_paths() code in that way, so 'admin' just becomes a special case of it.
In terms of the overlay module, though, I'm not sure what the use case would be for putting a non-admin path in the overlay.... The other way around (having some admin paths that don't appear in the overlay) I can definitely see people wanting, although at a certain point if you're trying to do that it would be better not to use the overlay module at all, rather just jQuery UI dialogs or something. I think the overlay module was pretty much designed to provide a consistent administrative appearance for all admin pages.
#4
could you elaborate on " .. designed to provide consistent administrative appearance "
it seemed to me the overlay can easily be turned around to be used for any purpose using hook_admin_paths or hook_admin_paths_alter .
for example you might want to display search results pager inside overlay, or even a lightbox gallery widget
regarding the patch, i took the easy way for creating the new hooks without need to change user_admin_paths and node_admin_paths, but the resulting hook_TAG_paths doesn't have a "good" name
so i am going to attach new patch that replaces all hook_admin_paths implementation in core .
#5
changed hook name from hook_TAGGED_paths to hook_tagged_paths
so that overlay for example implements overlay_tagged_paths($tag) instead of overlay_overlay_paths()
#6
Well, I was just thinking that the overlay module has tons of code (both JavaScript and PHP) whose purpose is to support the full admin experience taking place inside of it. It would be wasteful to load all that (e.g. even for anonymous users) if all you need is a simple lightbox popup. For that use case it would make a lot more sense to use jQuery UI or perhaps something like http://drupal.org/project/dialog instead.
However, if people are already using the overlay for site administration, maybe there are cases where they want to tweak a bit where it appears and make all their site's "overlays" look the same for everyone, so it definitely doesn't hurt to support this kind of flexibility.
#7
i don't agree the overlay is tons of code, i think it is equivalent of the modal frame and people should be encouraged to use it
it is not as simple to use jquery ui dialog as you describe it, for most cases it will require additional coding which can be reduced by simply using core's overlay
#8
I too am starting to use the overlay for end user facing UI that appears in the site theme (not the admin theme) this is another area where it would be helpful to separate admin paths (which receive the admin theme) from paths that open in the Overlay. For the most part though, I feel that the solution to this is not to use path matching but in my case I'm using some custom JS with classes on certain links to trigger an entry point. The path matching works better for admin pages, but less so for generic Overlay usage. (In my case, it's an commerce site where all product links open the Overlay).
As far as reason to use the overlay, there are tons: It's generally well tested, handles rendering subset of regions, has it's own template, handles back button navigation, and handles form submission (can be problematic in basic lightboxes). If you're requirements are similar it seems like a very obvious solution.
That said, I'm not sure about the patch in this issue, path matching is somewhat limited (I haven't tested to see if it works for aliases) and won't ultimately scale to thousands of paths that some folks may try to (ab)use.
#9
#1412776: Allow Overlay to be used outside admin screens is a related issue. Giving this one a slightly more descriptive title...
#10
Could this somehow be managed by hook_menu()? Maybe provide a "overlay path" parameter or something?
#11
I have marked #1412776: Allow Overlay to be used outside admin screens as a duplicate of this one.
That idea was considered back when the Overlay module was added, but it was abandoned for performance reasons (see comment #416).
I like the generalized approach rather than the duplication of the path logic used in patch in #1412776: Allow Overlay to be used outside admin screens. But I think I prefer hook_TAG_paths() over hook_tagged_paths($tag). The latter reminds me a bit about the $op argument for hook_nodeapi() etc. that was removed in D7.
In my patch I introduced a checkbox on admin/appearance that allows disabling the overlay for admin pages (without disabling the Overlay module). I think we should stick with this idea.
#12
it is true that every module implementing hook_tagged_paths($tag) will need an "if" structure to check the tag arguments which seems redundant , but i think although less "sophisticated" the "if" emphasize the relation between the module that defines the TAG and the module that defines the which path should be tagged by TAG , so it is better for readability of code and mode simple to use .
for example see the function overlay_tagged_paths in my patch .
#13
i also first thought of hook_menu when considering this issue and i think it is better implementation .
regarding performance , who said all information from hook_menu should be saved to a router record in menu_router ? the TAG information could be aggregated and saved to one array structure using variable_set
#14
Okay, let's stick to hook_tagged_paths($tag).
I think the “reference implementation” should use a
switch ($tag)/case/case/...construct rather than anif/elseif/elseif/.... This is in line with how hook_nodeapi() was usually made in D6.+function path_get_tagged_paths($tag = 'admin') {- $patterns['admin'] = implode("\n", $patterns['admin']);- $patterns['non_admin'] = implode("\n", $patterns['non_admin']);
+ $patterns[$tag] = implode("\n", $patterns[$tag]);
+ $patterns['non_' . $tag] = implode("\n", $patterns['non_' . $tag]);
This would mean that the semantics of path_is_tagged() would have to be changed slightly, but I guess we could make some kind of useful logic.
Instead of path_is_tagged(), would path_has_tag() be a better name?
I am wondering whether we could find a better name than “tagged” (to avoid confusion with the tags used in Taxonomy module), but I am out of ideas.
+ if ($tag == 'admin') {+ $paths = array(
+ // This is marked as an administrative path so that if it is visited from
+ // within the overlay, the user will stay within the overlay while the
+ // callback is being processed.
+ 'overlay/dismiss-message' => TRUE,
+ );
+ return $paths;
+ }
#15
generally i tried to preserve the original code logic as much as possible, this is why i didn't change the tagged paths array structure since that will require changing other overlay code .
you can off course suggest your own patch after making sure it passes the tests .