Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Following up on http://drupal.org/node/517688?page=1#comment-2172050, this is just the core API changes from patch 416. Overlay module code and CSS changes relegated to other issues.
Comment | File | Size | Author |
---|---|---|---|
#30 | 610204-overlay-api-changes-30.patch | 7.25 KB | Gábor Hojtsy |
#25 | 610204-overlay-api-changes-25.patch | 9.14 KB | effulgentsia |
#16 | 610204-overlay-api-changes-16.patch | 8.95 KB | effulgentsia |
#15 | 610204-overlay-api-changes-15.patch | 10.86 KB | effulgentsia |
#14 | 610204-overlay-api-changes-13.patch | 10.86 KB | effulgentsia |
Comments
Comment #1
ksenzeeDon'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.
Comment #2
sunok - 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.
s/something/obj/
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()?)
drupal_goto() needs to stay where it was -- otherwise, the message clashes with the 403 page.
We need some additional PHPDoc description here that explains when and why one would want to use this function.
Strange wrapping here. (should wrap at 80 chars)
I'm on crack. Are you, too?
Comment #3
effulgentsia CreditAttribution: effulgentsia commentedI 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.
Comment #4
seutje CreditAttribution: seutje commentedsubscribe
Comment #5
markus_petrux CreditAttribution: markus_petrux commentedsubscribing
Comment #6
Gábor HojtsyOn it.
Comment #7
Gábor HojtsyWorked 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.
Comment #8
Damien Tournoud CreditAttribution: Damien Tournoud commentedIt 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.
Comment #9
Gábor Hojtsy@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.
Comment #10
RobLoachHitting the "Awesome" button.
Comment #11
sun"Merge required query parameters for batch processing into those provided by batch_set() or hook_batch_alter()."
I think we can simply override $batch['url_options'] here?
"Return whether the given variable is an object."
Usually, we do not explain how to alter the variables - that's documented elsewhere.
I'm on crack. Are you, too?
Comment #12
effulgentsia CreditAttribution: effulgentsia commentedWith #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.
Comment #14
effulgentsia CreditAttribution: effulgentsia commentedThis has one more refinement to docs for toolbar.tpl.php to be more like comment-wrapper.tpl.php.
Comment #15
effulgentsia CreditAttribution: effulgentsia commentedAnd 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()?
Comment #16
effulgentsia CreditAttribution: effulgentsia commentedHm. 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.
Comment #17
Gábor Hojtsy@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.
Comment #18
Damien Tournoud CreditAttribution: Damien Tournoud commentedThe toolbar can easily been lazily rendered. Instead of:
... 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.
Comment #19
Gábor Hojtsy@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 :)
Comment #20
effulgentsia CreditAttribution: effulgentsia commentedGabor: 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.
Comment #21
moshe weitzman CreditAttribution: moshe weitzman commentedI 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.
Comment #22
mcrittenden CreditAttribution: mcrittenden commentedSubscribing.
Comment #23
sun#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.
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.
(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.
Comment #24
effulgentsia CreditAttribution: effulgentsia commentedI 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.
Comment #25
effulgentsia CreditAttribution: effulgentsia commentedWith the js change from #23.
Comment #26
eigentor CreditAttribution: eigentor commentedsubtype
Comment #27
effulgentsia CreditAttribution: effulgentsia commentedI 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.
Comment #28
marcvangend@#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".
Comment #29
David_Rothstein CreditAttribution: David_Rothstein commentedThe 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 :)
Comment #30
Gábor HojtsyUpdated 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 :|
Comment #31
effulgentsia CreditAttribution: effulgentsia commentedOk, at this point, I think this issue and patch has had enough eyes on it, that it makes sense to call it RTBC.
Comment #32
ksenzeeAgree RTBC. I'm rerolling the implementation patch now to reflect these changes, and they all seem sensible.
Comment #33
webchickExcellent. 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.
Comment #34
marcvangendYay! Nice work people!
Comment #35
David_Rothstein CreditAttribution: David_Rothstein commentedNote 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 :)