Given a context A, on which we have add a layer, called B. We registered a handler C for 'foo:bar' on A and a handler D for 'foo' on B. Handler D only returns data when the first argument is 'baz'. Handler C returns always data, regardless of the arguments
If we request 'foo:bar:baz' from B, in the current implementation, we get NULL as a result. This is however not a desired behavior, because C actually can return data for 'foo:bar:baz'.
After a lengthy discussion with Crell about this, we thought that is would be best to have the implementation follow the instructions described below.
If in the example above, we request 'foo:bar:baz' from B, it should check for handlers in the following order
- foo:bar:baz on B
- foo:bar on B
- foo on B
- foo:bar:baz on A
- foo:bar on A
- foo on A
If a handler is not found, it should check the next in the list. If an handler is registered, but it does not return data, the same thing should happen.
I wrote a patch for this, which is included in this issue.
But, this imposes another question. How should a handler indicate that it does not return data? In the implementation in the patch, it is assumed that if handler returns NULL, it does not return data. But according to Crell that is not a perfectly valid assumption, because in some cases NULL can be a correct value.
There are some ways to solve this. We discussed the first two of them at the codesprint in London.
- If a handler want to return no data, it returns NULL. If the handler really wants to return NULL as data, it should return some specific value. Pro: fairly simple implementation, Con: what should that specific value be, and how can we be sure that there never will be any handler that really wants to return that value for its data.
- If a handler wants to return no data, it should return some specific value. Same Pro's and Con's.
- An approach that we did not discuss in London, which I thought of today: The handler should throw an exception if it does not return data. Pro: really clean implementation and technically perfectly valid; we do not need to make up some artificial value that means 'no data' or 'really really NULL'. Con: some more work, both on the Context side and on the Handler side.
Any thoughts on this?
| Comment | File | Size | Author |
|---|---|---|---|
| fallthroug.patch | 5.5 KB | rolf van de krol |
Comments
Comment #1
Stalski commentedNice stuff.
For the handler returning no data issue, I am not sure this is a correct usage of an exception since nothing actually goes wrong.
I was wondering if we couldn't add a property indicating the state of that handler (like locked or some name more self-explaining) ? The object asking for the data could ask hasValue and act accordingly.
The patch did not work on current 7.x-1.x though.
- Edit -
Comment #2
Crell commentedThanks for the writeup, Rolf!
I don't think an exception works here either. Exceptions are actually rather unperformant since they always generate a complete backtrace dump internally. That would be terrible for performance, aside from the semantic misuse that Stalski mentions.
I'm still working to sort out the butler vs. wscci projects, so I'll look into this patch in more detail once I've gotten that straightened out.
Comment #3
xtfer commentedI considered this while patching another part of the module, and the conclusion I came to was an additional property indicating the handler state, as Stalksi has proposed. This could be largely ignored unless it was set to the equivalent of 'no value/NULL'.
Comment #4
Crell commentedRefiling to the sandbox queue...
Comment #5
Stalski commentedCreated a branch locally that includes the patch of Rolf as a start here. Do you prefer it as a patch. I followed the explanation at http://drupal.org/sandbox/Crell/1260830
If possible, I'll commit the branch.
Comment #6
Crell commentedYou've got commit access now. Push away.
Comment #7
Stalski commentedThe branch with the patch from Rolf is 1263682-context-variables-overriding.
Comment #8
Crell commentedImportant related discussion over here: #1261156: what does butler return when something is not there?.
Comment #9
Stalski commentedIndeed. Seems like one solution will almost fix the other too or at least provide an easier way of doing so.
Comment #10
Crell commentedPer today's meeting notes: http://groups.drupal.org/node/175544 I am marking this thread a duplicate of the other one. I think we're all agreed on the logic to implement here, but they need to be implemented together.