Context values: Keys or objects?

Keys would definitely be better for cache keys and static cache purposes, as you said, I definitely stick on that side.

But I'd propose a 3rd option:

• You don't store objects
• Caller can get $context['nid'] but it returns a "context helper", with two functions: getRawValue() and getNode()
• If context is an object, the context carrying $context['nid'] has a specific implementation with an helper function loadEntityNode() that does the node_load() itself and returns it

Pros:

• No business logic for cache key computing, just get all current context keys, sort them, and build a hash
• User doesn't have to type 3 lignes of code to fetch its object, and a lot of nice helpers can be implemented at the specific contextes level.

Cons:

• It creates a new layer in the API, maybe that's a good thing, maybe not, depends on how many of these context objects will be spawn during runtime probably, it's somehow code indirection
• User have to test for context object class at runtime before calling them

Example:

interface ContextInterface {
  public function getRawValue();
  public function getKey();
  public function __construct($key, $value);
}

class ContextNode implements ContextInterface {
  // ... Methods from ContextInterface
  public function getNode() {
    return node_load($this->_value);
  }
}

class ContextStack {
  /**
   * @return ContextInterface or NULL
   */
  public static function getContext($key);
  // ... Stack handling
  public static function hash($revelant = NULL) {
    // Some kind of static cache here would be good.
    $array = array();
    foreach (self::$contextStack as $objectHashcode => $object) {
      $value = $object->getRawValue();
      if (!isset($revelant) || in_array($revelant, $value)) {
        $array[$object->getKey()] = $value;
      }
    }
    ksort($array);
    return md5(serialize($array));
  }
}

// Somewhere when instanciated:
ContextStack::stack('node', new ContextNode('node', 251));

// Somewhere in user code:
if (($context = ContextStack::getContext('node')) instanceof ContextNode) {
  $node = $context->getNode();
}

// Other place, compute hash (for example a block relying on user and page only):
$hash = ContextStack::getContext(array('user', 'get:q'))

Couple of things here.

There are likely to be some context objects that you need to know the identifier but don't always need the full object. A classic example would be language - while we currently have global $language, 99% of the time in core, the only thing that gets accessed is the language code - $language->language for use in caching keys.

So there are three parts to this:

1. Determining the current context for a particular key - in some cases (maybe all?) this may itself be done when that's demanded (i.e. find me the current organic group for this node does not need to be front loaded).

2. Finding the current organic group based on a node should give you a node ID - at least entityFieldQuery + entity_load() enforce that kind of pattern and it's a good one. The node ID (or langcode or user ID) may be enough to generate a cache key just by itself - if that's all that's needed, then it'd be great to just do that. However it's possible cache key generation happens via methods on the context object, not by requesting context keys and figuring it out.

3. Lastly, there is requesting the actual object that the context handler is referring to.

If we decide #2 is only valuable for caching (and possibly very rare edge cases), then as long as the context system has helpers to generate cache keys that doesn't need to be available to code that requests the context object, so returning objects sounds good (although I'd still like to store only keys internally.

Because objects pass by handle in PHP, what you get back is a reference to the object in the EntityController class's static cache. Change that object and you're now changing global state outside of your scope, which we want to prevent.

On this, what happens if a function that requests context needs to trigger a node_save() or any other crud operation? The more things request context, the more likely this is going to become. Custom code, triggers, actions all do things like this all the time. In that case, you very much do not want later functions in the same page to be operating with a stale node object.

Currently changing objects without then _save()ing them is unsupported even though it's not enforced. This is part of #154859: Document that cached entity objects are not necessarily cloned any more, I'd want to see very, very detailed performance numbers on clone before going down that route, especially memory. Either way I'm not sure that very old discussion, which is nothing new, should affect decisions made here.

I originally posted that comment on the duplicate issue that Crell opened, hadn't seen pounard's when I wrote it.

Being able to get both the key and the value may be worth the extra method for callers. However I'm not sure why we need a special getNode() method, isn't something like this enough?

class NodeContext Implements ContextInterface {
  protected $nid;

  function __construct($nid) {
     $this->nid = $nid;
  }

  function ___get($name) {
     if ($name == 'id') {
       return $this->nid;
     }
     else {
      return node_load($this->nid);
     }
  }
}

$node_context = $context->get('node');

if ($node_context) {
  $nid = $node_context->id;
  $node = $node_context->value;
}

The magic __get() is supposed to be expensive, but I doubt we are going to be requesting context keys and values hundreds or thousands of times per request.

Just to clarify on clone:

1. the context class does something like $context['node'] = (clone) node_load($nid); and static caches this itself.

2. Some function reacting to a rule or trigger does $node->foo = 'bar'; node_save($node);

3. node_save($node); resets the static cache, but it will not reset anything for $context['node']; so now it's stale.

4. More unlikely, say another function does the same as #2, it's going to save a $node with stale data.

That seems like as likely a scenario as people doing $node = node_load($nid); $node->foo = 'bar'; then not saving it.

If nodes aren't a great example, presumably $_SESSION is going to be available as context at least potentially, and functions need to set or unset stuff in $_SESSION regularly.

But if we don't cache any objects in the context class, then this whole argument devolves to the various APIs that provide contexts, and that seems good to me.

@#3 Your code is pretty much what I explained upper, do not store obects, just set a proxy method to the real object factory.

The __get() method is a bit expensive, but it's not if you don't call it a thousand time. And by the way, PHP 5.3 did serious optimizations with function calling and stuff, so that tends to be not that true anymore (PHP 5.2 is officially unsupported now). What I mean here is that even if Drupal officially supports PHP 5.3, a lot of hosting will probably switch to PHP 5.3 so the performances should be targetted for this environment.

@#2 +1 for language, who needs the full metadata anyway? 99% of time you only need the language code.

If we decide #2 is only valuable for caching (and possibly very rare edge cases), then as long as the context system has helpers to generate cache keys that doesn't need to be available to code that requests the context object, so returning objects sounds good (although I'd still like to store only keys internally.

That's pretty much I'd propose the middle solution, that pretty much looks like your code. Plus, even if you don't have the helpers for spawning the node, considering that you are actually fetching the 'nid' key means that you KNOW what you are fetching and you know how to load it.

#6 for 1) and 2) The getter method seems transparent enough to allow something natural, if I take you code sample and try to adapt it with catch's suggestion:

class ContextValueRaw implements ContextValueInterface {
  protected $_value;
  public function __get($name) {
    if ('value' === $name) {
      return $this->_value;
    }
  }
}

class ContextValueNode extends ContextValueRaw {
  public function __construct($node_or_nid) {
    $this->_value = is_object($node_or_nid) ? $node_or_nid->nid : $node_or_nid;
  }
  function ___get($name) {
    if ('value' === $name) {
      return $this->nid;
    }
    else if ('node' === $name) {
      return node_load($this->nid);
    }
  }
}

$c2 = $c1->mock();

// For this I would prefer not to use ArrayObject, but magic __get and __set, because
// we could do: $c2->language = "fr" // And create default ContextValueRaw magically
// behind.
$c2['language'] = new ContextValueRaw('fr');

$c2['node'] = new ContextValueNode($some_node_object);
$t = $c2->lock();

$nid = $c2['node']->value; // Nid
$node = $c2['node']->node; // Node, if not the right instance, returns null.

// Could work the same for HTTP headers?
$langcode = $c2['language']->value; // String

Still for 1) The syntax, even with the value accessor seems straighforward enough, just need to be documented.

For 3) The mockup function would just be a deep clone of current context array object. I don't need to write a sample here because it's quite basic, but in this case clone keyword is probably best than mock() function.

$c1['language'] = new ContextValueRaw('fr');
$c2 = clone $c1;
$c2['node'] = new ContextValueNode($some_node_object);
$t = $c2->lock();
$nid = $c2['node']->value;
$node = $c2['node']->node;
$langcode = $c2['language']->value;
// Or for fun we could also do this implementing __toString() method, but would
// work only for pure string function, for the sake of consistency value property is
// probably better:
$langcode = (string) $c2['language'];

For 1), about the context propagation (language at node load time), you have three solutions:

• Let the user handle it (that's probably what we don't want).
• Create a context back reference in the context value itself (that should be replace at __clone time, and this same value object getter use it for loading.
• Even better, let the value object context back reference (with the same pitfall at clone time), make the node factory (the entity controller) aware of context and make context being an optional parameter of the NodeController::load($conditions, $context = NULL) method.

Sample:

class ContextValueRaw implements ContextValueInterface {
  protected $_value;

  protected $_context;

  public function getContext() {
    return $this->_context;
  }

  public function setContext(ContextInterface $context) {
    $this->_context = $context;
  }

  public function __get($name) {
    if ('value' === $name) {
      return $this->_value;
    }
  }
}

// Using the magic ArrayObject methods here could call implecitely the
// ContextValueRaw::setContext($this) method:
$c2['node'] = new ContextValueNode($some_node_object);

class ContextValueNode extends ContextValueRaw {
  public function __construct($node_or_nid) {
    $this->_value = is_object($node_or_nid) ? $node_or_nid->nid : $node_or_nid;
  }
  function ___get($name) {
    if ('value' === $name) {
      return $this->nid;
    }
    else if ('node' === $name) {
      return NodeController::getInstance()->load($this->_value, $this->_context);
    }
  }
}

// Would just propagate the context to the controller itself, that would test for
// revelant values to use for loading, such as language.
$node = $c2['node']->node;

I really don't like back ref, that's what made Views 2 almost impossible to debug, but in this case, if the code is clean and the design remains simple and is put on some diagrams for easy comprenhension, I don't see why we shouldn't use them.
This method ensure context propagation in the lower level API's and that's good!

To avoid backref, we could imagine something else:

class Context extends ArrayObject
{
  protected static $_current;
  public static function getCurrent() {
    return isset(self::$_current) ? self::$_current : new ContextNullObject();
  }
  protected static function _setCurrent(Context $context) {
    self::$_current = $context;
  }

  // ..

  // And in lock method:
  public function lock() {
    // ... do whatever is needed, then:
    self::_setCurrent($this);
  }

  //  ..
}

Then the NodeController could itself have:

class NodeController {
  public function load($conditions) {
      // Still be null if no language, but valid with no errors.
      $context = Context::getCurrent();
      $language = $context['language']->value;
    }
  }
}

Basically:

$value = $context['http:get:q']; 

Is exactly the same as:

$value = $context->http->get->q;

It's just a convention.

I'd bet that the right choice is the one that allows the more flexibility technically, but otherwise it's exactly the same. You can even implement both on the same object!

Value objects only allows to embed business method inside the context itself and provide a proxy pattern toward the business factories, but nothing mandatory here, it's not the more important part about it. I think the more important part is the fact that the context could propagate to the business stuff such as entity controllers themselves.

Some objects may need multiple IDs to load. Catch likes to use the nid-and-language example on a multi-lingual site. What does ->id return then?

Loading a node is not dependent on language - this is the case whether using entity-level translation (core translation module) or field translation.

So you would have $context['node']; and $context['language'] - two completely separate keys, not one combined.

Language is something that is determined either by querying a list of nodes restricted to a particular language or languages (Drupal 6 style), or rendering fields based on a langcode, so http://api.drupal.org/api/drupal/modules--node--node.module/function/nod... (Drupal 7 core style, which now co-exists with D6 methods).

In the former case the query will be affected based on language context. In the second example, node_view() requires language context (or optionally requires it depending on whether locale and other modules are enabled).

That doesn't mean there's a no valid use case for $id being an array, but node + language isn't it.

Yes I see, my point was more about "passing the context to the factory" in a generic way (not only for language itself, it could be for anything else, maybe some context restrictions or query rewrite).

Subscribing...

Just saying, but maybe at the early beginning it should remain simple, and should not provide any of business functions except __get() and __set() (or ArrayObject implementation, doesn't really matter). Let the consumer do whatever it has to do with the raw scalar values inside the context.

It would allow having a partial working context system really quickly, and business matter can be added later what it worth. Business will be useful only at the two "context usage endpoints":

1. When creating it (but we don't need any kind of helper here, the business layer would be able to do it itself so whatever);
2. At final usage time, where the consumer need the "nid" value. But the middleware layer doesn't really need to know what's a node or how to use it.

Hrm. I'm not really sure I really see either of the cons of option 1 as that significant:

- Because objects pass by handle in PHP, what you get back is a reference to the object in the EntityController class's static cache. Change that object and you're now changing global state outside of your scope, which we want to prevent.

Just return a clone of EntityController's static cache. Problem solved.

- We would still need to reduce the context information to primitives anyway in order to use it as a cache key, which we want to do for many reasons, including Varnish caching and smart cache invalidation.

The caching layer, I assume, will be invisible to people writing plugins, at least for the most part. Thus, it should be fairly simple to keep track of the id of the object requested internally, and afterwards, when accumulating keys to cache, ask each context handler thingy for the id by which they want to be referenced. Then, they can take this id to retrieve the full object.

I do, however, think that option 2 does have one advantage: when only the node id is needed, it's a waste to load the whole node. However, there are two possible ways I see around that. The first is to have it be possible to get $context['node'] and $context['node:id']. The second is something like this (this would probably be implemented on the entity level, and mitigates the first con mentioned of option 1):

class LazyNode {
  protected $nodeLoaded = FALSE;
  function __construct($nid) {
    $this->nid = $node;
  }
  function __get($key) {
    if (!$this->nodeLoaded) {
      $node = node_load($this->nid);
      // Do this for performance so __get doesn't keep getting hit.
      foreach (get_object_vars($node) as $node_key => $value) {
        $this->{$node_key} = $value;
      }
      $this->nodeLoaded = TRUE;
      if (isset($this->{$key})) {
        return $this->{$key};
      }
    }
    // Some sort of error -- key not found on node.
  }
}

Just return a clone of EntityController's static cache. Problem solved.

Whatever is the context, a business object such as an entity shouldn't be contextually modified. Clones are bad.

Whatever is the context, a business object such as an entity shouldn't be contextually modified. Clones are bad.

That applies to almost anything that's "contextual" though. The user object shouldn't be modified directly, but it's still used for contextual decisions. If something appears in context, it's for use in decision-making, not for modification/editing. I don't think that issue should affect the choice of key vs. object...

In a perfect world, clones may be bad, but in that perfect world, people wouldn't modify the objects returned by context. Unfortunately, we live in an imperfect world, which means that we need to solve these problems. In our imperfect world, putting a ->lock()/->unlock() method on an Entity object would significantly slow down all of Drupal, since all properties of all entities would need to be accessed with __get.

You're not even attempting to justify why we should do that just stating 'we must' and making assumptions. If the context system stores keys then it's not that system's problem to deal with the clone or not clone issue. And no one has answered what should happen if a function that takes context triggers an entity save.

> Just return a clone of EntityController's static cache. Problem solved.

Whatever is the context, a business object such as an entity shouldn't be contextually modified. Clones are bad.

That's how we handle it everywhere in Drupal since d7, so why should we handle it different here or change it again?

What would the downside of the approach I outlined in #15 be? When it's mocked, a different node object, filled out with fake properties, can be provided, which greatly simplifies it. It also provides a way for having either keys or actual node ids work.

If we wanted to get really complex, we could introduce some sort of tracker on the offsetGet method that would track whether the node that was gotten ended up filled out, etc., but that may be overcomplicating it.

@Crell I was referring to Dmitri's post immediately above mine insisting we should always do something wrong (clone everything) because sometimes people get things wrong. I should try to get out of the habit of responding to issue from my phone but I am getting fed up with that discussion both here and elsewhere.

$node = $this->service->getService('node')->load($this->context['node_id']);

This is closer to what I'm after, but I agree it's overly verbose, and I'm not sure it needs to be so verbose to get the same effect.

If we have a service locator, then what you've written doesn't look that far of what entity_load() does already - currently entity_get_info() has all kinds of dependencies on the module system but that doesn't necessarily need to be the case eventually (or for other context-providing stuff) - the config initiative and general clean-up should allow the entity info itself to be mocked or injected including which class is used to load it. In that case everything would have a dependency on the configuration system (and possibly plugin system if and when that exists), but those could be used to handle control otherwise.

Another option would be the context system allowing dependency injection directly:

So to get the id:

$nid = $context->nodeId;

To get the full node object:

// Ignore the method name.
$node = $context->getObject('nodeId');

When you need dependency injection, use a setter:

$context->setService('nodeId', 'my_node_load');
$context->getObject('nodeId');

getObject uses whichever service is set.

Internally it looks something like:

$context->nodeId = 1;
$context->services = array('nodeId' = 'node_load');

function getObject($id) {
  // Hopefully not like this but you get the idea.
  $function = $this->services['nodeId'];
  $function($this->nodeId);
}

If we could figure that out cleanly, then the user of the $context object isn't doing much more than accessing $context->node - the complexity is handled internally, and we provide methods to mess with it when people need to.

Its a bit OT, but in the process of building a registry module for D6 (http://drupal.org/project/registry), I've been paying close attention to developments in this thread. I expect the Alpha 3 release to have some semblance with the proposal above (the current Alpha 2 is a bit different), at least as far as the kinds of properties to be set and how to access them, and I mention it in case anyone is interested.

Is there any reason why getValue and getKey cant be implemented in the same class? For example...

class ContextHandlerExample extends ContextHandlerAbstract {
  public $id;
  
  public function contextKey(){
    return $this->id;
  }
  
  public function getValue(array $id = array()) {
    return example_load_function($this->id);
  }
}

This keeps the key and value implementation consistent. Otherwise we end up with contextKey implemented on the objects themselves, while the handlers requiring an extra class definition.

Okay, I understand 1:performance, however the performance overhead would potentially be quite small, and you would also be able start addressing concerns about modifying context values, loading objects not properly classed (like the current entity system), or situations where more information was needed about a string context (context is a string, but that string is part of a larger object/array)? Also, you could potentially use ContextHandlerExample::contextKey() without instantiating ContextHandlerExample, which would be quicker.

In the case of 2:DX, aren't you going to run into that anyway? contextKey implemented on Node or ContextHandlerNode still has to return node_load($id) either way (probably), and under the proposal in #24, the key and value accessors will be split across the handler and the object, which seems more complicated to me.

Additionally, it would be useful if Butler ever gets a workable 7.x release, as its unlikely that 7.x core will be reworked to accomodate the class implementation (my comments stem from an attempt to write the contextKey implementation into the Butler 7.x branch and having no reliable way to load objects like nodes for testing).

I think Im behind the times on static methods... I was under the impression they were faster but having done more research this is probably not the case, so ignore that.

Here's a patch which basically implements the functionality described in #24...

1) interface ContextValueInterface
2) A new method, contextKeys(), on DrupalContextInterface, which returns an array of keys for caching etc.

Additionally, there is an extra sub-module Butler Examples, to hold development related code not part of Butler itself. This contains an object implementing ContextValueInterface, for testing.

This is also available in a sandbox repository at http://drupal.org/sandbox/xtfer/1254810.

Whoops! That's sloppy. I've cleaned up the patch.

What we may need to know is the value of all context keys used in a given scope. For that, we'd probably need to track if a given key was accessed, and at what stack level. I think it would make sense that if we access key foo, then it is considered "used" by that context object and all of its ancestors. Then we can retrieve that data.

Given that keys are normally set when they are accessed via $context->offsetGet($offset), for any given request only those keys required should be set, which is roughly what the implementation in the patch at #32 assumes.

Variants on this...

a) Log the $offset in offsetGet then iterate over them in usedKeys(), or
b) Set the usedKey record in offsetGet directly and merely return the extant values in usedKeys, or
c) Assume that anything set has been accessed, and just iterate over that (i.e. #32).

I assume that (c) would be faster during the construction of the $context object, and (b) quicker when returning usedKeys. (a) is only faster if, for some reason, there are context values which aren't required for the request (but that begs the question what they are doing in context). (a) also has a downside that the key must be unset if the value is removed.

The exception I can see here is for context values set directly (i.e. via $context->offsetSet($offset)), do we assume that this has now been "accessed", or do we just ignore it till someone uses it – the problem here being instances where the object is used then set. Do we accomodate that (c), or filter them out till referenced (a or b).

Anyway, this patch implements (a) for comparison. Its missing the "scope" concept, it merely returns the current contexts keys (not its parents).

If thats the approach you were thinking of, let me know and I'll add tests.

Yes, I'd been considering that as well, though that shouldn't be too hard. Array of values? The caller could do its own filtering in any case. Tests forthcoming,,,

I also gave it a try, I did not see these new posts before.
I think the patches could be merged, since I did not do anything for the cached keys yet.

In this patch are included:
- A example object that is fetched from context through handler
- Overrides of the example object
- Problem with context requests not knowing that the contextKey was updated.
This is done in a page callback on butler-objects/1 and one simpletest function is added.

sub

The patch in #38 shows we might have a problem with requesting offset keys within context (keys).

I've added a simpletest function that exposes this problem in level 2 where a "id" property is requested while the id returned from level 1. So currently, if we don't explicitly set the offset key for a context key (E.g. example:foo), than we will get the wrong ID.

The question here is: Do we need to do load the properties of an object that is put into context?

Larry told me to create an issue for it, but this is a better place imo. The patch included is improved for testing purposes.
Thx for feedback.

I'll give it a try tomorrow morning. I do have to say I don't see much difference except that the patch in 40 includes tests.
So I'll take another look and try to understand what you meant. Probably the patch I tried is a too big a step.

I had the whitespaces as well, those should be fixed. Also added a closing ) in an if clause.

I added a implementation of the contextValueInterface as well as a handler that can take a parameter to mimic the loading of a example instance (E.g. load from url)

Most basic tests are included. I hope you can work with that tomorrow, have fun at the sprint ;)

@crell, cross-linking the promised issue: #1260320: Problem when requesting a property of an object from overridden context

Includes a patch that fixes the changed tests.

Good stuff. Thanks for working through the tests Stalski.