Provide a simple centralized context API

This may be a bit off-topic, but it gives my opinion about all the discussions that happend to get to this bug report, and what I really think of all of this.

That said, I agree with the base_path() problem; some stuff are definitely static and won't change, but I really don't see where the context doesn't fit. Get a component that generates URL, populate its internals with the context values only once, then use them. A static env. related variable is the exception and not the other way arround, and creating new mecanics just for those exceptions seems like over-engeneering.

As I understood it your problem is a matter of separation of concern: context API must not be invasive and must not end up as arbitrary procedural code in core's API for the sake of simplicity and decoupling.

The real problem, following your example, is that user_access() function must not accept and empty account because then it becomes dependent of the context API where it should NOT!

Context API doesn't bring this complexity, but it highlight it: actual core optionnal $account parameter is already too complex -keeping the same example- it's the real complexity.

Your concern is legitimate but misplaced: Context API does reveal the already existing design flaw but in any case doesn't add more complexity. It forces us to fix those consistency problems.

To sumarize the issue: core APIs (overall APIs) must loosely coupled where there are actually not: each bit of business API has its own business and shouldn't care by itself from the input data provenance. Context API is an INPUT helper and NOT a business helper. That's where the complexity lies, and that's where the hard work now has to be done. It's a matter of flexibility, performance, and maintenance.

Simplify the context stack has no goal at all in that regard. you indeed raised the right red flag, but I think you didn't spot the real problem cause. If you don't spot the cause, you don't guess the solution' And yes, this is a core issue, and definetely not related to context: let's not fork this work uselessly, and end up with a half-solution that would not fix the entire problem.

I guess that today, user and access procedural API is all about manipulating those globals.

This sounds really wrong. Think of it as an object oriented paradigm, I'm not saying "with the word class in it" because that would not make any sense, but as a set of functions that are related to a particular type of objects. Access and user related functions always needs an relative account on which to check access: I don't care you get those from globals or you pass it as a parameter, it needs it.

The problem that having a global account flagged as the current user doesn't make any sense at API level, it makes it when checking access at request time, but once the user got in, relying on it is spreading the need for the globals anywhere in the code: that confuses people because function calls then become magic and they forget what they really mean: having a mandatory account parameter is keeping the signature explicit and consistent.

Now imagine the menu becomes a front controller, the request object is a property of it: then it knows per definition which is the user to check upon, without the need of globals: the user_access() function can be safely called with the user instance.

Further, when it spawns a menu action (a page callback) imagine this is an object too: the request object will be passed to it as "here is your context, run according to it". Once again, the business callback that runs doesn't need globals anymore: it's part of its already set properties.

Now what happens in those, displaying a view, a form, etc, I don't care: those are business API, often business atomic API. The case of node_view() for example: from the request I got the context, from the context the path, from the path the derived node: where do I need any global in all that flow? And once I ran node_view() I don't need any context anymore.

Let's speak about the view too. The view object (as in the Views module I hate, yes) is an object: when spawning it from the action controller callback, you can pass it the request and the context thereof: Views have contextual arguments, how do I fetch them? Oh, it's already done! Where does it stand that *any* simpler API can be done here? It's a flow that already seems really simple: each object that spawns another gives to it the context it's in itself, and beside, the new object can add reduced-scope information for itself and its children from there, all remains encapsulated. I cannot see where any global, static, or simpler API can help us if that system works.

From that point, any object the view will spawn or need to manipulate will probably need those too: but because my view got all the data it needs, there is no need to go and fetch any globals or static anywhere. Now all data I need already are properties of my object (and if you're worried about IDE and code readability: I'd say that because the data is a set properties of my object, I cannot not see it).

The procedural drupal_get_context() accessor gets to be useful only when people needs it, let's say, into blocks for example, because blocks still are not part of this controller cascade algorithm yet. Now, imagine that all blocks becomes action controller, just as a the page callback, how do I spawn blocks? Typically, when altering the page (WTF) or something near that. What if my front controller is able to spawn the "page" object and gives it the original request and context: imagine the page object as a composite view pattern or derivative (or any other, PAC, MCV, don't care), it will naturally pass the context and request to all regions, then blocks it will spawn.

I didn't speak about any other more atomic functions, such as node_view(), node_load(), term_load(), user_access(), user_is_anonymous(), etc etc... Those are atomic API functions, and their scope is definitely reduced to their own execution only: they must remain encapsulated. There will definitely be a lot of debates arround entities view (why don't I have a context when I render my entity) and other details, yes, there is a lot of stuff to discover and figure out, but for it worth, my opinion is that for now, having a simpler API for the logged in user definitely is not a good idea, because in the cases we still didn't figure out how: you still have the drupal_get_context() function (and I hope their will be some others, like drupal_get_request() or drupal_get_current_controller() or such) as a temporary solution, that definitely acts pretty much like fetching the global.

I got your point, I love refactor, indeed. Hum the user_access() argument is solvable in many ways (even some that doesn't involve changing its signature):

• Let it exists as-is, but relying a simpler context API (what you propose) so context goes out of scope
• Let it exists as-is, but use the procedural context accessor (what WSCCI actual patches does)
• Fix it and change its signature to force the $account parameter (what I propose)

Out of 3, 2 are definitely not invasive for core, and are relatively quick to do.

user_access() is a good example, because it's a function that on most pages you are not supposed to call thousands of time, so the performance impact of 3 function call is insignificant, as long as modules uses it wisely. But I guess they usually don't, hence the concern.

user_access() is a good example, because it's a function that on most pages you are not supposed to call thousands of time, so the performance impact of 3 function call is insignificant

?!?

user_access already takes an account to operate on. If none is passed, it defaults to the current user. Solving problems we don't have? http://api.drupal.org/api/drupal/modules--user--user.module/function/use...

I think #6 sums up the logic here fairly well, actually. Without replacing random globals with a clear, controlled pathway for injected information we cannot adapt to a modern web-services-based model, which is the goal of WSCCI.

"A solution should be as simple as possible, but no simpler". The WSCCI Context API is, I believe, already "as simple as possible" to solve the use case specified by the WSCCI roadmap. It is more than we need if all we're doing is s/global/$GLOBALS/, but that is not all we're doing.

Moshe: What pounard is saying is that the "grab the global user variable and assume that's the default value" logic in user_access() is itself a flawed design in the first place. He's right.

If we were designing an API from scratch, I think it would be best to make it a required parameter. "Does user X have access to thing Y?" That's a self-contained operation that has no reason to rely on global data.

I'm not suggesting necessarily that we make user_access()'s second parameter required right now. That's just an example. However, moving more of Drupal further in that general direction is a necessity if we want to support cleaner, unit testable, partial-page-is-easy web services and page rendering, which is what WSCCI is targeting.

I'm not suggesting necessarily that we make user_access()'s second parameter required right now.

Why not?

Mostly because I was talking in general terms about API concepts, not specific details of specific functions. If someone submitted a patch to make it required, I would not object to it. (Hence the "right now" part of that sentence. :-) )

Still more IRC discussion. :-)

I don't like forcing the caller to have to explicitly pop the context object, as one person forgets and the context elsewhere in the system is all jacked up. However, I am open to exploring ways to better separate the context object and stack object.

So something like this:

$stack = context_stack();
$c2 = $stack->addLayer();
$c2->changeStuff();
$t2 = $c2->lock();
do_stuff(); // uses $c2 internally.
// $t2 dies, stack pops $c2.

However, that still leaves the problem of values defined on a previous context object but not on the "active" object. That requires either:

1) The context object knows its parent and calls up. (What we're doing now.)
2) You don't call the context object, you call the stack object, and it has the call-up logic in it. (This is a fair number of additional method calls, and a significant API change.)
3) We clone the context object outright. That involves a fair number of internal values to deep clone, which would be non-trivial. We considered using clone() but rejected it a while ago as it didn't seem worth the trouble.

chx asked me to post this here. This is a naive, haven't-tried-running it conversion of comment_permalink() to use the context overriding mechanism (as it exists in the wscci sandbox as of right now) rather than mucking about with superglobals:

function comment_permalink($cid) {
  if (($comment = comment_load($cid)) && ($node = node_load($comment->nid))) {

    // Find the current display page for this comment.
    $page = comment_get_display_page($comment->cid, $node->type);

    $context = drupal_get_context();
    $c2 = $context->addLayer();
    $c2->setValue('path:system', 'node/' . $node->nid);
    $c2->setValue('http:query:page', $page);
    $t2 = $c2->lock();

    // This will run with the alternate $c2 context as "active".
    return menu_execute_active_handler('node/' . $node->nid, FALSE);
  }
  drupal_not_found();
  // $t2 goes out of scope, and $c2 is destroyed.
}

Advantages:

1) No mucking around with super globals, which is bad form anyway.

2) We know for certain that none of our changes will get percolated up above this function. That means we can safely use this function even in places other than where the original author intended, and won't accidentally change a global out from under us we didn't expect.

3) While it would be nice to always pass the context object rather than using the stack for out-of-band communication, it's unrealistic to expect every function in Drupal to do that any time soon. Maybe in a couple of years we can revisit that question. :-)

The context knows its stack only for being able to spawn trackers which are aware of it. They can have no stack (potential use case of the Context API as a custom business helper).

This may looks circular, but Context instance never use directly the stack, it's not depending on it. Stack knows contextes we put into, but doesn't mess up with them.

I choose this fix in order be able to:

1. Use contextes without a stack;
2. Use naturally the tracker, i.e. lock the contextes, without having to know about the stack existence, each new context inherit the stack of its parent. Notice that the context doesn't pass the stack to the Tracker, the tracker gets it: the Context never have to mess up with the stack, it only gives it to children when creating them end of story;
3. It makes the code and the interface in sync. Without this fix the stack was not part of the interface and the actual code was in desync with the interface (Context was messing up with the stack and the Tracker had to call the Context destructor directly: this is a really BAD behavior), it's unsafe and unreliable code. Having the stack accessors being part of the interface allow the Tracker instance to rely upon the interface itself without worrying about the implementation.
4. EDIT: And this way of optionally injecting the stack using direct interface defined accessors instead of having a fixed static entry also allows to use another ContextInterface implementation if we want to, as long as it implements the interface right.