Support from Acquia helps fund testing for Drupal Acquia logo

Comments

ksenzee’s picture

Status: Needs review » Reviewed & tested by the community

Don't know if this is kosher, but webchick already reviewed all of this in #517688: Initial D7UX admin overlay and was fine with it, so RTBC. I've applied it to a fresh install and clicked around, and I've read through the code again to make sure it's all just independent improvements and bugfixes that have nothing directly to do with overlays.

sun’s picture

Status: Reviewed & tested by the community » Needs review
+++ includes/batch.inc	20 Oct 2009 11:31:34 -0000
@@ -109,11 +109,20 @@ function _batch_progress_page_js() {
+  // Add ?id=x to the batch URL. Retain the existing query value in URL options.
+  $url_options = $batch['url_options'];
+  if (isset($url_options['query'])) {
+    $url_options['query']['id'] = $batch['id'];
+  }
+  else {
+    $url_options['query'] = array('id' => $batch['id']);
+  }
@@ -189,7 +198,18 @@ function _batch_progress_page_nojs() {
+  // Add ?id=x&op=y to the batch URL. Retain the existing query value in URL
+  // options.
+  $url_options = $batch['url_options'];
+  if (isset($url_options['query'])) {
+    $url_options['query']['id'] = $batch['id'];
+    $url_options['query']['op'] = $new_op;
+  }
+  else {
+    $url_options['query'] = array('id' => $batch['id'], 'op' => $new_op);
+  }

ok - so here is what I don't understand: In both cases we set or overwrite the 'id' query parameter.

So why don't we just set it?

$url_options['query']['id'] = $batch['id'];

...works and is the same for both cases.

+++ misc/drupal.js	20 Oct 2009 11:31:35 -0000
@@ -344,4 +344,11 @@ Drupal.theme.prototype = {
+Drupal.isObject = function(something) {
+  return (something !== null && typeof something === 'object');

s/something/obj/

+++ modules/shortcut/shortcut.admin.inc	20 Oct 2009 11:31:35 -0000
@@ -519,19 +519,25 @@ function shortcut_link_delete_submit($fo
+    $link_path = preg_replace('/\?.+/', '', $_GET['link']);

This line could use a comment that explains what is being done here. (I don't really grok the regex -- are we just removing everything behind the path? If so, why not simply using parse_url()?)

+++ modules/shortcut/shortcut.admin.inc	20 Oct 2009 11:31:35 -0000
@@ -519,19 +519,25 @@ function shortcut_link_delete_submit($fo
+    if (shortcut_valid_link($link_path)) {
...
+      drupal_goto();
     }
     else {
       drupal_set_message(t('Unable to add a shortcut for %title.', array('%title' => $link['link_title'])));
     }
-    drupal_goto();
   }
   return drupal_access_denied();

drupal_goto() needs to stay where it was -- otherwise, the message clashes with the 403 page.

+++ modules/toolbar/toolbar.module	20 Oct 2009 11:31:35 -0000
@@ -36,16 +36,33 @@ function toolbar_theme($existing, $type,
 /**
+ * Enable or disable the toolbar, or find out the current status of the toolbar.
+ *
+ * @param $enabled
+ *   If given, will change the display mode of the toolbar to the given
+ *   boolean.
+ * @return
+ *   The current status of the toolbar, TRUE for enabled, FALSE for disabled.
+ */
+function toolbar_enabled($enabled = NULL) {
+  $setting = &drupal_static(__FUNCTION__, TRUE);
+  if (isset($enabled)) {
+    $setting = $enabled;
+  }
+  return $setting;
+}

We need some additional PHPDoc description here that explains when and why one would want to use this function.

+++ modules/toolbar/toolbar.tpl.php	20 Oct 2009 11:31:35 -0000
@@ -6,15 +6,17 @@
+ * - $classes: Classes for the toolbar div element. Can be
+ *   manipulated in preprocess functions via $variables['classes_array'].

Strange wrapping here. (should wrap at 80 chars)

I'm on crack. Are you, too?

effulgentsia’s picture

I agree with sun's review, but am not familiar enough with the needs of overlay that prompted these changes to implement all of his suggestions with sufficient confidence. Leaving it to someone else to do that.

seutje’s picture

subscribe

markus_petrux’s picture

subscribing

Gábor Hojtsy’s picture

Assigned: Unassigned » Gábor Hojtsy

On it.

Gábor Hojtsy’s picture

Assigned: Gábor Hojtsy » Unassigned
FileSize
10.77 KB

Worked on updated patch based on @sun's suggestions:

- shortened url options array manipulation as suggested
- renamed something to value (NOT obj, because we don't know whether it is an object, that is the point of this method)
- modified preg_replace() to explode(), since the given 'link' is a path plus an optional query string without other actual URL parts, so looked pointless to use full-blown parse_url() - which could have been misleading IMHO
- moved back drupal_goto(), even actually doubled that, which let me get rid of one of the duplicate dsm()s on unable to adding shortcut; shorter code
- added more phpdoc on toolbar_enable(), also referencing the batch API case, which has its own bug report at #563562: Batch API pages should not show the toolbar. This new function will be useful there too, so cross linking that issue here too.
- adjusted wrapping in $classes doc

Don't think there were any other issues. Tested this patch manually, the dashboard was broken, but it was totally broken without this patch too, so unrelated. Did not find any other issues.

Damien Tournoud’s picture

It makes more sense to me that the toolbar defines its visibility based on the context (batch API or in overlay), rather then the other way around. I'm pretty sure that there are other cases where it is useful to be able to hide the toolbar (so toolbar_enabled() should stay), but the docblocks are misleading, IMO.

Gábor Hojtsy’s picture

@Damien: maybe in the batch API case, we have batch API as a core API, and toolbar as an optional module, so we can make toolbar know about the batch (but that already has its own separate issue, so let's not solve that here). In case of overlay, we deal with two optional modules. Whether the toolbar will know about overlay or the overlay will know about the toolbar and ensure compatibility probably depends more on preference I'd say.

RobLoach’s picture

Hitting the "Awesome" button.

sun’s picture

+++ includes/batch.inc	21 Oct 2009 08:55:26 -0000
@@ -109,11 +109,15 @@ function _batch_progress_page_js() {
+  // Add ?id=x to the batch URL. Retain the existing query value in URL options.
@@ -189,7 +193,13 @@ function _batch_progress_page_nojs() {
+  // Add ?id=x&op=y to the batch URL. Retain the existing query value in URL
+  // options.

"Merge required query parameters for batch processing into those provided by batch_set() or hook_batch_alter()."

+++ includes/batch.inc	21 Oct 2009 08:55:26 -0000
@@ -109,11 +109,15 @@ function _batch_progress_page_js() {
+  $url_options = $batch['url_options'];
+  $url_options['query']['id'] = $batch['id'];
@@ -189,7 +193,13 @@ function _batch_progress_page_nojs() {
+  $url_options = $batch['url_options'];
+  $url_options['query']['id'] = $batch['id'];
+  $url_options['query']['op'] = $new_op;

I think we can simply override $batch['url_options'] here?

+++ misc/drupal.js	21 Oct 2009 08:55:27 -0000
@@ -344,4 +344,11 @@ Drupal.theme.prototype = {
+/**
+ * Check if the given value is an object.
+ */
+Drupal.isObject = function(value) {

"Return whether the given variable is an object."

+++ modules/toolbar/toolbar.tpl.php	21 Oct 2009 08:55:27 -0000
@@ -6,15 +6,17 @@
+ * - $classes: Classes for the toolbar div element. Can be manipulated in
+ *   preprocess functions via $variables['classes_array'].

Usually, we do not explain how to alter the variables - that's documented elsewhere.

I'm on crack. Are you, too?

effulgentsia’s picture

With #8 and #11.

Regarding #8: Do we really need toolbar_enabled() as a function? When would it not be better for a module to implement hook_page_alter() and set $page['page_top']['toolbar']['#access'] to FALSE? I left it in, since I don't have enough history with this patch to know for sure, but it seems like a gratuitous API function to me. But I changed the docs to be more inline with the feedback in #8.

Regarding #11: Yes, we do document the use of preprocess for $classes in every tpl file. See node.tpl.php. I changed this doc to be more like the others though. Also, we don't usually add 'clearfix' in the preprocess function, but instead in the tpl file, so I changed the code accordingly.

Status: Needs review » Needs work

The last submitted patch failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
10.86 KB

This has one more refinement to docs for toolbar.tpl.php to be more like comment-wrapper.tpl.php.

effulgentsia’s picture

And this one fixes an incomplete fix to #11 item 2. If this one passes testbot, before setting to RTBC, please see #12: any reason not to remove toolbar_enabled()?

effulgentsia’s picture

Hm. I decided to reverse the question. Here's the same patch without the addition of toolbar_enabled(). I believe there needs to be a convincing use-case for it to be added. The overlay module wanting to turn off the toolbar in some situations is what hook_page_alter() and #access is for. If there's truly a good use-case for a toolbar_enabled() function, I'll be happy to re-upload #15.

Gábor Hojtsy’s picture

Status: Needs review » Needs work

@effulgentsia: the idea is that we do not even build a possibly expensive menu based tool for the page, if it is going to be disabled later. The idea is that we stop the toolbar before it is being built, not disable it after it took all the resources to be built. There were some performance optimizations in terms of the underlying menu API (the toolbar was slower before), but IMHO it could still be valuable to skip generating it when we know we don't need it.

Damien Tournoud’s picture

The toolbar can easily been lazily rendered. Instead of:

function toolbar_page_build(&$page) {
  if (user_access('access toolbar')) {
    $page['page_top']['toolbar'] = toolbar_build();
    $page['page_top']['toolbar']['toolbar_drawer'] = isset($page['toolbar_drawer']) ? $page['toolbar_drawer'] : array();
  }
}

... just return a proper renderable array, and move parts of toolbar_build() into a #pre_render function.

The poor initial design of the toolbar is not an excuse to introduce even more poor code in core.

Gábor Hojtsy’s picture

@Damien: instead of reinventing the wheel, we modeled toolbar_enabled() after @sun's excellent admin_menu's admin_menu_suppress(). Thanks to your ideas, looks like we can do even better :)

effulgentsia’s picture

Gabor: Thanks. That's what I was wanting. If the answer is performance, I'm all for introducing some way to optimize. Do we have any well-established pattern in D7 for when to lazy-build vs. when to lazy-render? My one concern with relying on lazy rendering too much is that any new child elements that get built during the #pre_render stage aren't accessible to site builders in hook_page_alter(). I sent Moshe a PM asking him to chime in.

moshe weitzman’s picture

I like the #pre_render suggestion. We should use it for slow code that might be removed in page_alter(). Seems like a good use case here.

@efflugentsia points out that it might be harder for someone to alter the output using this technique. But remember that one can always add their own pre_render that runs right after the one here. That accomplishes the same goal, and gives us late building.

mcrittenden’s picture

Subscribing.

sun’s picture

#pre_render sounds good to me as well. An approach like that wasn't possible in D6, because we didn't really have hook_page_alter().

Additionally, that exact same #pre_render callback could cache the rendered toolbar content in cache_menu (which is what admin_menu is doing as well). Whenever the menu is rebuilt, that cache will be flushed automatically.

+++ misc/drupal.js	21 Oct 2009 23:49:59 -0000
@@ -344,4 +344,11 @@ Drupal.theme.prototype = {
+Drupal.isObject = function(value) {

Looking at this once again, I wonder why we don't extend $ instead, since this function seems to complement jQuery's $.isFunction(val)?

Since it seems that jQuery developers planned this helper function anyway, we would just remove it from drupal.js, once jQuery provides its own.

+++ modules/toolbar/toolbar.tpl.php	21 Oct 2009 23:50:00 -0000
@@ -6,15 +6,23 @@
+ * - $classes: String of classes that can be used to style contextually through
+ *   CSS. It can be manipulated through the variable $classes_array from
+ *   preprocess functions. The default value has the following:
+ *   - toolbar: The current template type, i.e., "theming hook".

(Should not hold up this patch) I'm not sure who decided to put this info about theme hook processing into the docs for each variable, but obviously, we would have to repeat the very same information for every single variable in all templates again - which is quite... nonsense.

We can leave this in here for now and clean that up in a separate issue.

This review is powered by Dreditor.

effulgentsia’s picture

Status: Needs work » Needs review

I added a separate issue for lazy building the toolbar as a performance optimization: #612974: Optimize toolbar to only build if it will be displayed. Work on overlay should be able to proceed regardless of whether or when that optimization is accepted.

So, I'm setting this (patch 16) back to "needs review". This patch includes minimal API changes (a function added to drupal.js and a 'url_options' added to a batch definition), so hopefully can be accepted even if API freeze has already happened.

effulgentsia’s picture

With the js change from #23.

eigentor’s picture

subtype

effulgentsia’s picture

I would consider #25 to be RTBC if it weren't for the change to the way isObject() is added in drupal.js. That was a change I added in response to sun's feedback in #23, but it hasn't gotten any review. Can someone please review that and set this issue to RTBC or provide further feedback? Thanks.

marcvangend’s picture

@#27: I can't look into Sun's head (probably wouldn't understand all the ingenuity going on there anyway :-)), but I think your change to isObject is the only way he can have meant "extend $ instead".

David_Rothstein’s picture

Status: Needs review » Needs work

The changes to the shortcut module contained in this patch don't really make sense.

If we're going to disallow query parameters from shortcut links, we should check for that inside shortcut_valid_link() rather than in a menu callback, and we shouldn't bother adding the query parameters in shortcut_page_build() in the first place if they are only going to get stripped out later on.

Also, the issue with query parameters is really a separate bug with the shortcut module, not specific to the overlay. I just created an issue for it at #614498: Add handling for query parameters in shortcut links. So I think it would be best to just remove all the changes from shortcut.admin.inc from this patch.

Otherwise, the patch looks pretty good, at least based on a quick glance :)

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
7.25 KB

Updated patch without the shortcut.admin.inc changes as per David. Noting that breaking out these issues to their own will make the actual overlay implementation dependent on a growing list of issues though :|

effulgentsia’s picture

Status: Needs review » Reviewed & tested by the community

Ok, at this point, I think this issue and patch has had enough eyes on it, that it makes sense to call it RTBC.

ksenzee’s picture

Agree RTBC. I'm rerolling the implementation patch now to reflect these changes, and they all seem sensible.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Excellent. Awesome work on the reviews on this, folks. This removes my WTFs from my earlier API review, and I'm comfortable committing this to HEAD.

marcvangend’s picture

Yay! Nice work people!

David_Rothstein’s picture

Note that the followup patch for dynamic theme-switching at #553944: Define hook_menu_get_item_alter() as a reliable hook that runs before the page is doomed is also a required API change for the overlay... reviews there are welcome :)

Status: Fixed » Closed (fixed)
Issue tags: -overlay

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