I fail to see how an API which is $context->get('foo:bar') is going to be useable at all -- how will you have any idea of what foo:bar is available and what they are doing? I know I am going to be shut down with "you created $form["#foo'] so shut up" but a) it wasn't actuall me b) it was six years ago c) we are supposed to learn.

Comments

chx’s picture

I have tried to talk about this many times but it falls apart every time.

  1. eaton points out the scatteredness of context.
  2. we decided to solve the problem. It's a solvable problem. It'd make Drupal much more awesome and simpler. Rare treat.
  3. Suddenly instead of focusing on the context problem we have a full blown rewrite of Drupal at hand.

What we have at http://groups.drupal.org/node/67583 is a feature list. There's no mention of things like easy to document, easy to discover and so on. These are features that Drupal remarkably lacks and we are set on to create another API which is like that?

If there ever was a discussion on hashing out the API, I missed it.

pounard’s picture

It's something that has landed as-is since the beginning, and I never read a concrete discussion about this yet. My opinion would have been to allow context to have business methods, and modules could provide their own context implementation to provide explicit methods, but I admit this goes against the all-extensible stuff.

But discoverability is indeed a problem with this model, relying on documentation is always a problem; we know that, no matter how hard core is well documented, if code doesn't speak by itself we are going to lose developers (lose some that will go and work with other software, lose others that just won't understand and won't be efficient).

That's why somehow I'd like the API growing to be more granular and expose more stuff than just the getValue() method, at least it would allow developers to go beyond that and discover stuff pragmatically.

Crell’s picture

Given that context values are all lazy-derived for performance and memory reasons, how would you suggest we make it easier to detect what context values are available? Calling hasValue() on random strings is no improvement over just dpm()ing something and seeing if it breaks. :-)

chx’s picture

So suddenly this is my problem to solve? :( I am not sure I have good solutions -- but for example instead of a getValue have perhaps properties on the objects that context handlers can populate with the name of their factories so like ${context->http}()->q ?

Crell’s picture

Welcome to Drupal development. ;-) If I knew a good answer I'd just do it.

I'm not sure I follow your example, though. By design, asking for 'http:query:page' could come from http:query:page, http:query, or http, and as a caller you don't and shouldn't know which it is. So $context->http->query->page wouldn't work, because you don't know if the handler is on http, query, or we have a literal value on page.

Properties would also require using __get() magic methods, since they don't exist until they are first accessed.

pounard’s picture

I'm not sure I follow your example, though. By design, asking for 'http:query:page' could come from http:query:page, http:query, or http, and as a caller you don't and shouldn't know which it is.

Designed as-is or not, not giving public discovery and introspection methods is an error. A lot of developers will try to bend the API to do what they want, and it's best giving them enough to do it in a proper way that not giving them anything and forcing them to bypass and do ugly stuff in the end.

Crell’s picture

As I said, I'm open to suggestions on better documentation/discoverability as long as it doesn't violate the basic premis on which the context system is designed (that you request information and do not know or care where it comes from).

This really should have been brought up over a year ago when we worked out this model in Copenhagen... :-(

pounard’s picture

Adding method that allows you to bypass the original high level design does not violate it as long as most people can still use it the way it was created originally. I still don't understand why you don't want to be a fully featured API.

chx’s picture

So we have a problem here. We want to add everything dynamically, that's the nature of Drupal but IDEs and other development tools are a static code analysis tools. IDEs aside, one thing that could happen here is that we add our own @whatever to the context handler classes (? not sure) and document what foo:bar they could return? Perhaps what the parent is? Something along those lines? Then API module can just discover what we want?

chx’s picture

Another thing here, how much can the code discover itself? So if i say 'devel mode' can it figure out what could possibly be available, basically switching off the on-demand mechanism?

Crell’s picture

What we can introspect from any given context object (and therefore, presumably, it's parent) is:

- At what keys there is a literal value assigned, or a value has already been derived by handler.
- At what keys there is a handler assigned.

The caveat is that the key where a handler is assigned may not be the key that it actually responds for. Vis, the http handler is assigned at "http", but it will respond for http:query, http:request_args, http:cookies, http:query:foo, etc. It's that complete list that is problematic to generate.

I think the http handler is probably abnormal in how many possible keys it will respond to. In practice, I suspect most will be far far simpler. I am not certain of that, however.

Crell’s picture

In IRC, we threw around the idea of introducing a new docblock tag that handlers could use to specify the keys that they expected a handler to be bound to. It wouldn't be a guarantee, but it's a statement of where the author expected them to be bound. So you'd have:

@suggested-key path:system

on HandlerPathSystem. The few compound handlers, like http, would specify multiple:

@suggested-key http:query
@suggested-key http:request_uri
@suggested-key http:request_body
...

(@suggested-key is a terrible name for this tag, but it gets the point across for now.)

Documenting this here for discussion. It may be a good idea or not. :-)

neclimdul’s picture

It would easily be parseable and we could build lists and doc pages based on it so sounds reasonable.

We should include a 1 line description with the key. I think the compound handler example clearly shows why....

chx’s picture

Well, see there's http://drupal.org/files/1262014-path-handlers.patch $httpProperties if THAT would be a property which name never changes we would be a lot ahead. Can we do that?

Crell’s picture

That property is actually vestigial. We should probably have removed it, as a default case on the switch statement would work just as well. Actually looking over that handler again there are a couple of issues I need to fix with it. :-)

HandlerHttp is not going to be a typical case, I think. Most handlers will respond for one key, or that one key plus arguments on it. Very few, I think, will respond for the wide range that Http does. The only reason that's in a single handler at all, rather than a half-dozen, is to avoid duplicating the $request object.

So no, that's not really a good model to follow.

catch’s picture

Couldn't individual handlers call back to a single request object?

In the HTTP handler issue people are talking about keeping the old procedural wrappers like request_path() and current_path() - which I would not be very happy with at all. If we're going to have a unified context API, we should not keep a tonne of procedural wrappers laying around just because the API itself is either overly verbose or hard to discover.

So what about registering dedicated handlers for the most commonly requested keys, then leave the http handler itself when you really need to inspect the request (in which case what you're looking for comes down more to the spec than anything else).

Crell’s picture

The procedural wrappers are not staying, worry not.

As currently written we could not split up the http handler without duplicating the request object. Which technically doable, that seems like a bad idea to me, especially when that's perhaps the object in the system most likely to only make sense to have once. It's also more memory usage for, I think, little value.

catch’s picture

I don't get this - aren't the two path handlers having to access the http handler as well (note I've not reviewed the code from that issue yet)? I'd object to duplicating the request object just to do this, but that sounds like an implementation detail, not to do with the API concerns expressed here.

webchick’s picture

Project: WSCCI » Drupal core
Version: » 8.x-dev
Component: Miscellaneous » wscci

Per catch, and blessed by Larry, moving this and all other WSCCI issues to the Drupal core queue.

cosmicdreams’s picture

Forgive me for bringing up a Microsoft technology here but this is how I've been thinking of the Context initiative thusfar. This discussion has me wondering if my assumptions are correct.

When we talk about the "request object" do we mean something roughly analog to: http://msdn.microsoft.com/en-us/library/system.web.httprequest.aspx ?

Likewise when we talk about context do we mean something roughly analog to: http://msdn.microsoft.com/en-us/library/system.web.httpcontext.aspx ?

It would be great if our context object were able to expose this kind of information on page request. I think that bringing all of this information into one object would greatly improve the ability to discover what's going on the page that is currently being processed AND the previous page that called it. I would be able to debug a Drupal site with an IDE and put a breakpoint down, watch this object, and monitor how it changes as I step through code. That's the kind of thing that would improve my comfort with the Drupal codebase immensely.

Is that the kind of discoverability we're talking about here?

Also, it's pretty awesome to think about what modules could do with that kind of information.

Crell’s picture

Both of those libraries map roughly to the HttpFoundation library's Request class, which is what we're now using. I don't believe there is a direct equivalent elsewhere for the Context object as we're building it, as that encompasses much more than just the HTTP values but also things that derive from it. See http://groups.drupal.org/wscci/definitions

webchick’s picture

Category: bug » feature

The context API is not in core yet, so this can't possibly be a critical bug against core. Moving to a critical feature request instead.

Crell’s picture

Category: feature » task

webchick: That raises an interesting question of how we file issues in the wscci component against the wscci sandbox that do not screw up the core counts. Because the wscci sandbox absolutely can have critical bugs that need to be fixed.

Refiling to task, since there is no feature here.

aspilicious’s picture

In irc I talked about adding an @group for context. That way we can have a summary like: http://api.drupal.org/api/drupal/includes--module.inc/group/hooks/7

We could add to each handler:

/**
* @addtogroup context
* @{
*/

/**
* Description for getValue()
*
* @contextkey path:system
* This is a short description or the group page
*/

/**
* @} End of "addtogroup context".
*/

pounard’s picture

Putting a tag over getValue() method is dangerous, because it's relative to the key the handler has been set to in the context instance.

It's a start I guess.

aspilicious’s picture

You're right... hmmm

Crell’s picture

That's basically what was suggested up around #12.

catch’s picture

Priority: Critical » Normal

wscci isn't in yet, so this can't be critical, nor even major, and the discussion should probably be merged in to the main context API thread.

@Crell. Lots of patches against core have critical bugs in them that need to be fixed, but we deal with those in the issues working on them, I don't see how it needs to be different if the code lives in a sandbox.

Crell’s picture

catch: This was marked critical when this issue was back IN the sandbox. We moved all open issues to the core queue at your request. But if the result of that is we cannot use critical and major for fear of polluting the "real core" numbers, then we're being hamstrung in our ability to use the tools we have. At that point I'm inclined to just go back to the sandbox, which was working just fine and is perfectly discoverable (except for people who only look at the "all issues on core" feed as their only source of tickets, who are vanishingly few).

catch’s picture

Why would you move an issue back to the sandbox, when it is a review of #1233232: Add unified context system to core, which is currently a core issue?

catch’s picture

Status: Active » Closed (duplicate)

In fact, why do we have two issues open, I'm marking this as duplicate of #1233232: Add unified context system to core, and have linked back from that issue. If there are unresolved bits of the discussion in here, then we can move them over but a lot of the same issues have been discussed in that issue as well.

Crell’s picture

I meant in general, not this issue in particular. If wscci-sandbox-code issues being in the core project queue instead of the sandbox project queue means we cannot use categories and priorities, then it's not a worthwhile tradeoff.