| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | base system |
| Category: | feature request |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (duplicate) |
| Issue tags: | WSCCI |
Issue Summary
One difficult thing about writing code for Drupal is that it can be very difficult to tell anything about the current context. Because the systems are completely separated from each other, often the only recourse that a piece of code has is to use the URL to try and sniff out context. This works, but is ugly and looks an awful lot like hardcoding in a way that isn't beneficial to future growth.
One example is a very common piece of code:
<?php
if (arg(0) == 'node' && is_numeric(arg(1))) {
'...'
}
?>This piece of code is everywhere, especially in PHP snippets. It's a piece of code I would like to get rid of. With this patch, that piece of code would change to:
<?php
if ($node =drupal_get_page_context('node')) {
...
}
?>This patch is the basis of a system that could fix that. I am submitting just the base for now, because I'd like to see discussion on actually utilizing the mechanism before spending any time going through the code and applying context information. The piece of code presented is nothing more than a fancy way of doing a global variable without polluting the global namespace. Well-named functions are easier to find and document, but otherwise, it is a very simple storage mechanism.
Drupal should define context appropriately amongst all its systems. The obvious are page and node, but other possibilities are taxonomy, book, forum...whatever seems logical. Contrib modules can use this to declare their own contexts -- Organic Groups, in particular, could benefit a great deal by this by being able to set the 'og' context to the group, making it much easier for snippets and block code to determine whether or not they should appear.
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| page_context.patch | 1.62 KB | Ignored: Check issue status. | None | None |
Comments
#1
Nice and simple, I like it.
#2
I'm setting this for review; I believe the appropriate way to do this is, if we agree this is the way to go, to commit this patch and create additional issues for each area where page context would be set.
The really obvious two are $user and $node objects and should come right away, but a few others could follow. We do have to be careful though. We should consider:
1) Do we want to have context proxies, so that we don't have to have fully loaded objects always? This is fine for nodes that are cached; it's fine if users are cached too. I forget. In that case, you'd only store the NID or UID and rely on node_load and user_load to get the object. This makes it easier to put other items into the context lazily without burdening the system loading items that may not be utilized.
#3
A couple of other obvious contexts:
book --> the parent book that the existing node lives in. The context quickly tells you it's a book and what book we're looking at. (Then book navigation could change how it discovers it needs to appear).
#4
From a theming side, this is *extremely* important. This is the basis for many complex themes I write and I'm constantly checking the $node object (using node_load(arg(1))) to check the node type, and then do something. Or check the $user and do something else. I use this all over the place and have written some static functions to do something very, very similar.
+1e100 ;-)
For a great example of a module that uses this, OG does this with: og_get_group_context(), see: http://cvs.drupal.org/viewcvs/drupal/contributions/modules/og/og.module?...
Follows very similar format that Earl is proposing.
After this goes in, we'll need to figure out *where* to set various contextes, but I think having that as additional patches makes sense, with first patch being $node and $user.
#5
Based on the +X of killes and m3avrck I'm going to RTBC this and see if Dries and/or Steven agree.
#6
yup - will be useful
#7
Yeah - I like the idea of this - not 100% sure if this is the perfect approach, but I can't think of a better one, and this would certainly do the job!
There was an error - I believe that it should be:
<?phpfunction drupal_get_page_context($variable = NULL) {
$context = drupal_set_page_context();
...
?>
...instead of the (nastily recursive):
<?phpfunction drupal_get_page_context($variable = NULL) {
$context = drupal_get_page_context();
...
?>
Edited patch attached.
#8
I'm still not convinced that we actually need this.
There are two issues with this, IMO:
1. It's push vs pull -- we're greating a pull system, which can have unnecessary overhead. We're putting stuff in the context without using it. And because we don't know whether it will be used, we'll have to be conservative and always add it.
2. It's just like global variable -- buth then using functions instead of PHP's super-globals.
#9
Dries:
1) We have to define how it's going to be used. And...what are the alternatives? I can't think of a way to do a pure 'pull' system, because module foo doesn't know anything about module bar, and can't go query module bar to see if module bar has put up a simple $node. If module bar pushes the $node into the global context, though, anyone can see it, regardless of whether module foo is acquainted with module bar's API or not.
2) It is, in fact, just like a global variable. We could, in fact, implement this using global variables, but we've already expressed interesting moving away from global variables in order to avoid namespace pollution. In fact, I addressed this in my original post:
The piece of code presented is nothing more than a fancy way of doing a global variable without
polluting the global namespace. Well-named functions are easier to find and document, but otherwise, it is a very simple storage mechanism.
So if your criticism is that it's just like a global variable, I need more than that to respond to it. Because I've already admitted and told you it's just like a global variable. In fact, as far as I can tell, you're agreeing with me, but you're saying this is a bad thing without telling me why, even after I just told you why I think this is a good thing.
#10
Let me reiterate, I used this on *every* single site I build. OG uses the same thing.
It's clearly needed :-)
This step would make it easier to theme and could lead to all sorts of interesting themes that actually start to do more sophisticated things with less theming.
#11
As a themer, I have to say I think this would make the barrier to entry on using template.php much lower, and anything that can make the mildly-php-savvy stay is (imo) probably a good thing as long as its overhead is minimal. That and making the template.php easier to read would be a good thing for both maintenance and tutorial sake. I won't pretend to understand all the issues involved here, but I will say that based on my minimal argument above this appeals to me a great deal.
EclipseGc
#12
Let me also reiterate:
In the documentation for the context, we will place the following rule:
"Do not add anything to the context that is not already loaded for some other purpose. Try to place IDs and not whole objects (and use caching, like node_load uses, to ensure utilizing that ID doesn't result in extra loads)."
#13
Not only that, but core will provide contexts for nodes, blocks, and users so it will be up to modules to follow suit. I imagine views, panels & OG are 3 very big contenders to reuse, not sure about other ones.
#14
We were talking a little about this on IRC. I am not naming the guy who posted this summary, he can himself.
#15
I have to agree with Dries. Global contexts are a big hammer for what seems a very small nail. They have also been brought up for specific things (like filters) and have consistently been rejected.
I think that there is in fact very little reason for centralization here. The idea of contexts sounds nice until you realize that in spite of all the context data being grouped together in a central APi, no single module will in fact ever iterate over that entire store of data (other than maybe for debug purposes): it simply wouldn't know what to do with the data. Any module that uses this will just cherry pick what it needs from the available values. That's warning bell 1.
I also agree with Dries' push vs pull comment: this API requires every module to ensure the right context is always set (i.e. calls to drupal_set_context spread across Drupal). So you either need overzealous loading and caching of context data, or you need a bunch of calls to drupal_set_context() spread all across your modules. This is not easy at all... a lot of things in core are loaded and rendered only at the very last second and lots of data is discarded after use. Contexts would change this. Warning bell 2.
The current patch is just two unused API functions, which looks nice and compact... but we need to factor in how this should and would be used, and I think it would be messy.
Because of that, it seems what we really need is just a fast way of getting the right data for each situation.
For example, what if you could replace the following snippet:
<?php$node = (arg(0) == 'node' && is_numeric(arg(1))) ? node_load((int)arg(1)) : NULL;
?>
with:
<?php$node = node_load();
?>
Same for user_load(). This is really quite simple to implement.
This follows the principle of "do what I mean" which has been successfully applied in things like Rails or jQuery.
This would also kill the use of the $user global and help avoid people accidentally screwing with their $user object / session.
#16
I don't like the idea.
The concept of internal path in Drupal is 'basic' and powerful. It should be something that even a themer should understand : what a node is, what a block is, and... what an internal path is, and how to read it, and how to NOT confuse it with any alias the path may have.
One would always need to know the internal path, then one can break it down into... arguments.
The only thing I don't like about arg() is the name of the function, actually. And the terminology 'argument'. The function looks unfriendly, it looks like a geek function that only hackers ('in the know') use.
#17
For themers it would certainly be a great addition.
Right now it is a little tricky to have different layouts for different contexts.
Anyway, I always thought the context should have been stored in the $page variable so that I can switch this way:
<?phpif ($page ='view') {
...
}
if ($page ='node') {
...
}
if ($page ='node-foo') {
...
}
if ($page ='comment') {
...
}
?>
But probably it is not codewise...
#18
Drupal's concept of internal path is indeed powerful and basic. It is not, however, sufficient. A perfect example is views module -- one of the most populate modules in all of Drupal contrib. Its paths are not based on a predictable view/$vid style, but can be mapped to any legitimate path without the use of aliases.
In addition, custom paths exposed by modules, dynamic paths that display different content based on the current user, and so on can't be 'contextualized' using just the menu path. These cases are NOT hypothetical edge cases, they're common uses of some of Drupal's most important modules. If you feel this patch's approach is less than ideal, that's one thing. But reliance on the menu path is insufficient for any advanced site.
#19
First of all, I think this issue is extremely unclear.
Some people want every module to provide $node objects, other people want it just as a replacement for (arg(0) == 'node' && is_numeric(arg(1))) (and would probably get undesired side effects from the first behaviour). Some people think this is about classifying every single Drupal page into a single Taxonomy of Drupal Page Context (which is not the case).
Whatever it is, this patch is not ready by far to be committed because it is just a dead getter/setter now that is not being used. If we intend for global context to be created, we need clear use it in core and a clear, defined list of items that can go into the context. Simply providing a magic box for contrib modules to fill out is not an acceptable solution and will only lead to wildfire. If context is the choice, then by all means, code a patch that provides this rich context information for core and shows how it can be used effectively and how centralization helps, where simple wrappers fail.
This patch doesn't really address the push vs pull issue even. There would be a huge difference conceptually and performance-wise between calling drupal_set_context() after doing a bunch of checks in your _init() hook, vs doing some specific calls in specific menu callbacks that deal with nodes/users/...
There are also a lot of implicit timing issues here: page.tpl.php comes after blocks comes after main content. For speed, we'll want context setting to happen in the page view, and not in some _init hook. But if we do that, it means a lot of module code cannot rely on context being available, and the rules for this will not be immediately apparent (and may even different between different invocations of the same hook).
As for more use cases, IRC came up with the following (which I don't all agree with as being valid use of Drupal):
#20
Interesting issue. Currently I don't know enough Drupal details to participate in the discussion. Subscribing.
#21
The more I think about this the more I think that we might be able to do something with the new D6 menu framework, since the arg values (i.e. the current menu path) form the guts of the context, and what we really want to be doing is just layering richer information on top of this.
What I can't quite wrap my head around is that while this seems to be a good place for an API to clearly define what should be provided (e.g via a callback) it is a useless place for actually adding useful information to the context itself (since that will generally be figured out later, in the guts of the module).
I'll have to think about this some more...
#22
#23
did this get in? either i missed it, or this issue is not 'fixed'.
#24
Automatically closed -- issue fixed for two weeks with no activity.
#25
please explain how this is fixed.
#26
The idea is that this is fixed by the addition of auto-load functions in D6's menu system. I disagree, as I think context is more than just arg(x).
#27
I like to think that the context system in Panels demonstrates that it is significantly more than just arg(x).
#28
I've added a module to contrib called 'context' that I'm using on every buildout I do, and that I'm sure others will find useful - http://drupal.org/project/context It's very similar the original patch above, but implemented slightly differently and adds namespaces.
I'd like to see something of this sort in Drupal core, but it's unclear how best to go about that.
#29
@jmiccolis - thanks for contributing that module. looks quite useful.
#30
+1
This has been at the back of my mind for ages (same for nodes too: #259897: Context data for nodes), though I've only just found this issue...
It would definitely make a huge difference to theming if the page template knew what sort of thing it was showing. Designers continually request things that require this: different formatting for views, weird things done with page titles, and so on, and themers end up having to mangle the theme system to accommodate them. And as has been pointed out above, panels and view and potentially other things in future can be anywhere in the path structure.
(It springs to mind that we could have views and panels canonically live at "view/ID" and "panel_page/ID" respectively, and use aliases to put them somewhere nice: to use the same approach as node module in other words. I tend to give views a path at "views/ID" until I know where they need to go.)
But instead of the global-like approach, why not return more than flat text from the page callback?
So we'd have this:
function mymodule_menu() {$items['mypath'] = array(
'title' => 'My module page',
'access arguments' => array('access content'),
'page callback' => 'mymodule_page',
);
}
function mymodule_page() {
// generate content here
return array(
'content' =>'Some content we made here',
'context' => array(), // some information such as: module name, content ID, anything else that may be useful.
}
This is similar to the way blocks work, so it's not a huge conceptual change.
(And -- potential patch creep -- we could return a title here too and simplify drupal_set_title perhaps?)
#31
This sounds like something for the WSCCI initiative.
#32
WSCCI has taken over this admirable feature. See http://groups.drupal.org/node/198538