Don't use ArrayAccess

Is there a reason we can't just dictate that context keys cannot have colons? # has a specific meaning in FAPI and that's never been an issue (aside from people forgetting about them, but that's a separate issue. :-) )

Object properties cannot contain colons either, so that wouldn't be any improvement.

OK, yeah, PHP... :-) Still, I think I'm fine with just saying that key components do not have colons and calling it a day.

I've been wondering about this despite my love for ArrayAccess, it's a shame about the string parsing this entails.

I am trying to follow all the WSCII twists and turns with some difficulty. After reading the above, I must ask:

So with WSCII, how will we deal with context/keys like $context['https://www.example.com/foo'] or $context['url:http://www.example.com/foo']?

Lars: You will never have a context key of "http://www.example.com". That is not a context key. $context['http:scheme'], $context['http:request:domain'], etc. are context keys. http://www.example.com is a value.

There's little if any practical difference between $context->offsetGet('foo:bar:baz') and $context['foo:bar:baz']. The latter is simply syntactic sugar over the former; I rather like it myself, but if that sugar ends up being too costly performance wise we can eliminate it later.

That is a separate question from the keys being colon-delimited. We determined back in Copenhagen that we wanted "nestable" context keys, as that gave us an enormous amount of flexibility and allowed us to reuse handlers. (No sense attaching a different handler to every http:get:param property.) It's nominally string parsing, but it's about as simple a string parse as you can get and the only alternative would be asking all developers to pre-split their keys into an ugly array, which makes the call to get a context value considerably uglier and more cumbersome. We want that API to be really really easy to use, so that people actually use it.

Thanks @crell. I had understood that it would be possible to specify both the key and value in $context[].

How would that be any different with just a method?

if ($context->hasValue('foo:bar:baz')) {
  $baz = $context->getValue('foo:bar:baz');
}

if (isset($baz)) {
  // Because $baz is NULL
}

isset($c['foo']) and $c->hasValue() would be exactly synonymous, and the code flow identical. Ibid for $c['foo'] and $c->getValue().

As for ArrayAccess being "ugly", that is perhaps a personal style. :-) I rather like it, as it results in fewer characters and lets you view the context more naturally as a "collection of keys that have values". It would hardly be the first ArrayAccess in core, either. Both the DB layer (or rather PDO itself) and the new CacheObject use it already. It has really no effect on code flow at all, as you have the same number of methods that behave in the same ways either way.

With methods it'd be possible to add equivalents of both array_key_exists() and isset(). The issue with ArrayAccess is array_key_exists() just doesn't work at all.

We've been able to skip over that for DrupalCacheArray because that's primarily designed to be hidden behind procedural code (and hence easily backportable). That means contrib and custom module developers never actually get to see the SchemaCache or ThemeRegistry classes themselves (if they're using DrupalCacheArray for their own stuff that's a different level of integration to calling theme or schema functions).

If we refactor the schema or theme registry to OOP fully, we might want to use a similar caching pattern, but we might well not want to use ArrayAccess.

I think there's two things being discussed here though, $context['foo'] vs. $context->get['foo'] - for the 'root' context keys, then the foo:bar:baz syntax. chx's original suggestion in this issue was Drupal::context->foo->bar - if each level is using __get() then it can return either a class with methods or a property so you'd still get chaining, but it'd be clear that each level is an object as opposed to actually being a string array key.

So theoretically any of these ought to be implementable:

$context['foo']->bar->baz

$context['foo']['bar']['baz']

$context['foo:bar:baz'];

$context->foo->bar->baz

However I doubt anyone is going to support either of the first two, so that leaves the two original examples here.

This also ties into the config API a bit - the same issues with nesting, chaining etc. apply for http://groups.drupal.org/node/155559

However in that case you only get to do $config = config('foo.bar'); the $config itself is whatever value the config system allows (which at that time was only going to be arrays).

So if that still stands, we're looking at two similar syntaxes with some very subtle differences:

$config = config('foo.bar.baz');
$config['foo']['bar'];
// Or potentially.
$config->foo->bar

vs.

$context = context();
$foo = $context['foo:bar:baz'];
// $foo could be anything - string, array or object here.

Now they don't need to, and can't, be exactly the same. Config is using the periods because that maps to file names and there is strict namespacing, but why periods vs. colons? Not using strings for namespacing at all would hopefully mean less opportunity to get confused.

$context->foo->bar->baz is not even remotely equivalent. To implement the fallback logic we already have implemented using that syntax would require several more objects, some crazy data passing, and I'm not even sure if it's possible. That would only be an option if we totally gutted the entire design and started over from scratch, which is not on the table. $context['foo']['bar']['baz'] is even less viable as an implementation.

Config API is also not really comparable. Config API's original hope was to be able to do "break at any point" within a single tree of values or value-objects. Thus, it would simulate a single unified tree accessible via $config->modules->views->cache_enabled (or whatever) and it would just kinda work behind the scenes. That idea was dropped in Denver as unworkable. (The "it just kinda works" part, well, doesn't.) The most recent design, I believe, uses a colon-delimited addressing to a specific file on disk, which is itself just a serialized object. (All of the lengthy debate over file format was really just deciding on an alternative to serialize(). Really.) The context addressing here does not map to files. It's more equivalent to menu paths, but with only postfix parameters.

I don't know if there's any special reason we ended up with colons instead of backslashes or periods or whatever. Whatever the separator is we end up with, that separator will not be valid in a key name (which was the original point of this issue). I don't see a reason colons don't work, since we don't have this sort of key elsewhere so there's no pattern to follow. (The exception would be / for menu paths, but I don't want to go there because it would confuse people with menu paths, which context keys are not.)

So the only viable syntaxes that wouldn't require completely changing the design would be:

1) $c['foo:bar:baz'];
2) $c->get('foo:bar:baz');
3) $c->get(array('foo', 'bar', 'baz'));

The third one there makes me twitch just reading it. I'd hate to have to see that scattered around Drupal.

The first still strikes me as the easiest to work with. In practice I don't know that NULL will come up all that often. In fact I'm rather hoping that individual keys will have something contextually meaningful for "empty" (like an empty array) rather than NULL. I'm more concerned about the potential performance difference, as ArrayAccess does add overhead on the function call. Whether or not it will be negligible at the macro level I don't know (nor do I know how we can test that until we've implemented it one way or another).

Hmm I can see how $foo->bar->baz potentially makes it even more complex.

I've not benchmarked offsetGet() vs. a straight get() method, would be interesting but is likely to be negligible.

I'll have a look at the config code and see if it's switched from periods to colons, if that's the case then it's consistent (doesn't matter either way to me, but I think we'd need a good reason to have two different delimiters here).

I benchmarked ArrayAccess back in PHP 5.2 here: http://www.garfieldtech.com/blog/benchmarking-magic

I ran the tests again in PHP 5.3 more recently, and while everything was faster overall I think the ratios were roughly comparable.

If the separators in the Config API and Context API meant the same thing, I would absolutely agree that they should be the same separator. However, they don't really behave the same way. I am concerned that using the same separator but with different semantics will be more confusing than using two different separators.

And I misspoke about Config API, I see. It's not using colons, it's using periods as of when I last looked at it. (Colons in file names, while nominally legal, are a major PITA.) That's what I get for typing late at night.

I'm on beejeebus side on saying that the context behavior actually isn't the same than arrays, and the ArrayAccess interface will definitely confuse about normal context usage.

Also agree that extensive __get() usage would trigger highly complicated code and object trees.

I'm all in flavor for a single normal OOP accessor method get($someValue), accompagnied with a hasValue($someKey) method: magic does not always solve problem, it's pure syntactic sugar as Crell said, but it may also strongly hide what's really happening behind.

OT: You surely do no like long method names, get() instead of getValue(), I don't see why get() only is better: but whatever, either it's has() and get() or it's hasValue() and getValue(), but I would definitely go for the second choice since it's more explicit.

We could also have a 4) looking like this:

4) $c->get('foo', 'bar', 'baz');

No need to build an array beforehand. Not sure it brings anything useful except reducing the typing.

More importantly, there's a Developer eXperience issue here: using methods, we have the advantage of normal IDE and doccing built-in documentation/intellisense kicking in, which does not happen with ArrayAccess pseudo-keys.

I definitely agree with #20 about the fact that decent editors will gracefully do autocomplete and builtin documentation with "normal" methods: Drupal is evolving and core is bigger and bigger every major version. It's even more important when switching to OOP where all code start to become contextual and encapsulated: the need to highlight the design by using clear method names become really essential (the shorter are the names, the more you will confuse them with others in another objects in another contextes) plus the decent IDE help become really useful.

If ArrayAccess does not work 100% exactly like native arrays, it'll end up being a DX problem rather than an advantage. My 2¢.

The switch to OOP will already confuse some people, making it even harder with "strange" and "usual" syntax can confuse even more (I'm still trying to understand how $context["foo:bar:baz"] works.) Normal getters look fine for me.

subscribe

subscribing

so there are discussions to better integrate ArrayAccess with array_*() but they are fairly non concrete at this point. This may or may not result in BC breaks .. so in other words no clue what will happen :-/

http://screencast.com/t/fKkFn4sD

Based on the discussion here and the results from http://groups.drupal.org/node/179154, we will be dropping ArrayAccess. The new API, per today's IRC discussion, will be:

1) offsetGet($context_key) becomes getValue($context_key)
2) offsetSet() becomes setValue().
3) registerHandler() becomes setHandler(), for consistency.
4) offsetExists() goes away entirely.
5) Colons will not be changing at this time as that's out of scope for this issue.

I will update the summary momentarily, and will be working on this issue.

And changes have been committed. I've attached the effective patch for reference.

Added summary of the decided direction. Crell is on the job.