Current Context class does access to stack directly, this cause multiple problems:

  • Unneeded complexity
  • Dependency to static properties
  • Even if design is not meant to, by the fact that an interface exists, module could override and use their own specific implementation at some point: we should make the implementation easier and failsafe

The stack only exists for procedural code to be able to get the current context, it's not part of the design, it's a helper, the Context object itself only need to know what's its parent is, but doesn't need to know anything about the stack.

Additionally, the ContextTracker class purpose's is the same as the database transaction objects: it tracks the current context scope: instead of cascading tracker destructor to call context destructor, it should directly interract with the stack itself.

Cleaning this and move the complexity to the most outer edge of the API would benefit for the code being used as a pure API: additional modules could use the Context objects outside of the stack for pure business purpose without having to reinvent the wheel. That purpose is not the core's one right now, but I think that shipping this kind of API with 8.x would definitely encourage module developers to reuse core code instead of doing their own.

My proposal to get a cleaner design is pretty simple, and needs these steps, which can be achieved incrementally, in order:

  1. Move stack from static to instance, add it to methods: push(ContextInterface $context) and pop(ContextInterface $context) for direct manipulation, and the final (what we are actually seeking) getActiveContext() method. This needs of course to set it up as a singleton as a start. We can still make the Context class manipulating this new singleton from this point.
  2. For this to work complete, is that we need to inject a context's parent when building a Context instance, for it to be able to call its parent directly without manipulating the stack: this needs a setter in the the ContextInterface interface, such as setParent(ContextInterface $context). For internals, it may, or may not have a getParent() method, but IMHO it should as soon as we do implement the setter.
  3. Inject the stack instance in root context (using setter injection so people who don't need it just don't set it), we get rid of the global state and singleton at least in the API perspective. From this point, we can get rid of the singleton definitely: Context instance will inject the stack to the ContextTracker instances at the moment it creates it, only if it has one set.
    Keep a reference of new non static ContextStack and linked Context instance into the ContextTracker sot the tracker can push() and pop() in or from the stack at construction and destruction (algorithm remain the same).
    From this checkpoint, we need to create the stack instance at bootstrap time, and set it as a static property of the procedural accessor, the only place it should remain.
    By the fact that injection is not mandatory, the whole API can run without stack, any developer that needs it can use it from this point.

This design is more complex, by design, because of the new dependency injection, but it has many improvements:

  • The whole API can work without a stack (context only needs to know its parent).
  • It reinforce the ContextInterface position inside the full design, it makes it a first-class citizen and even more important than the underlaying Context class implementation, and it definitely becomes pluggable.
  • It gives a new flexibility for contrib modules developers, which can use it as an API and not only as the core context stuff.
  • Definitely removes the global state and static stuff.
CommentFileSizeAuthor
#14 1297142-stack-static-removal.patch11.71 KBCrell
#12 global_context-1297142-12.patch1.47 KBcsevb10
#12 full-1297142-12.patch11.17 KBcsevb10

Comments

pounard’s picture

pounard
Primary language French
commented

I see at least one case where the getParent() as a public method in the interface can prove itself to be useful later: to do a nice debugging UI that shows the stacked contextes.

The null object context implementation is a must, as soon as we make an interface and an extensible mecanism, even if we do not use it right away, we need to provide a null implementation for testing and debugging to developers: it's exactly the same use case as the DrupalNullCache, and it should be applyed to almost everything that can be backendable/pluggable.

Furthermore, if we have the ability to make this method, we definitely should do it it's an API bonus, even if we don't have use cases right now, module developers will find some pretty soon: better provide a fully featured API than a really strict and limited API, it will provide a longer life time anyway and make a lot less future developers angry.

Crell’s picture

Crell commented

We absolutely do not want to expose getParent() as a public method, as that breaks the encapsulation and defeats the entire purpose of the context API.

I don't know what you mean by a null implementation. new \Drupal\Context\Context() returns an "empty" context object with nothing on it, which you can then use for testing to your heart's content.

pounard’s picture

pounard
Primary language French
commented

A null object implementation serves many purposes:
1. You can identify it's a stub because by testing with instanceof: you can then identify the root context by checking if its parent is a null object.
2. All methods are hardcoded which removes all the complexity: calling methods over a null object won't mess up with internal array's and be a thousand time faster (hardcoded null and false return for everything).
3. Setting per default null object everywhere we have to allows us to do safe call chaining instead of bringing code complexity with if() everywhere at a reasonable cost.

pounard’s picture

pounard
Primary language French
commented

We absolutely do not want to expose getParent() as a public method, as that breaks the encapsulation and defeats the entire purpose of the context API.

I believe we don't have the same definition of encapsulation. It's encapsulated since the internals are hidden from the outside world, it doesn't mean we should not provide a full API: developers are adults and crazy: we'd better give them a fully featured all purpose API with usage conventions than a poor and non flexible API they won't be able to extend.

pounard’s picture

pounard
Primary language French
commented
Status: Active » Closed (won't fix)

Oups.

pounard’s picture

pounard
Primary language French
commented
Status: Closed (won't fix) » Active

Updated the description.

pounard’s picture

pounard
Primary language French
commented

First commit, more or less checkpoint 1:

  • Moves the stack into the ContextStack class, which becomes a singleton instance.
  • Removed some stack logic from Context class to ContextTracker else the root context dies when destructed.
  • Context $parentId property is now the $parent Context instance instead.

That's pretty much it, all tests pass except the one about throwing exceptions when parent is not in the stack, but it's better this way: it's not the Context class itself that should check that anyway.

See full diff there: http://drupalcode.org/sandbox/Crell/1260830.git/commitdiff/047d57ab8a8b1...

pounard’s picture

pounard
Primary language French
commented

Now, added the implementation of checkpoint 2:

  • Introduces getParent() and setParent() on ContextInterface, so the interface is now constructor free, and easier to extend.

See the #7 following diff there: http://drupalcode.org/sandbox/Crell/1260830.git/commitdiff/7a68cdb43b4ff...

pounard’s picture

pounard
Primary language French
commented

Implemented the check point 3:

  • Made setStack() and getStack() method being part of ContextInterface, now ContextInterface is a first-class citizen of the API instead of being a useless tool. The Context class is now fully consistent with the ContextInterace
  • Removed Context direct class usage pretty much everywhere it can be found (maybe some occurences remains). The default implementation is now fully encapsulated, and can be overrided by any class that implements the ContextInterface
  • Removed the singleton pattern inside the ContextStack class, and moved the singleton instance inside the drupal_get_context() method instead: the OOP API is fully static free.
  • Moved all stack handling into Tracker instance: Context instance does not needs more than having working accessors, the stack logic does not belong to the Context class or ContextInterface interface anymore.

See the incremental patch from #8 here: http://drupalcode.org/sandbox/Crell/1260830.git/commitdiff/4bff41fa730b5...

Crell’s picture

Crell commented

Can we see a patch here as well as gitweb links? That's much easier to review.

pounard’s picture

pounard
Primary language French
commented

Your eyes and an editor which your are comfortable with definitely are the best tools. Sorry for being sarcastic but don't have much time right now to offer you much more than that, you'll have to wait a few days if you really need a patch.

csevb10’s picture

csevb10 commented
Status: Active » Needs review
StatusFileSize
new full-1297142-12.patch11.17 KB
new global_context-1297142-12.patch1.47 KB

I've attached 2 patches here:
1. A patch against wscci branch
2. A patch against 1297142-stack-static-removal branch.

My changes (the global_context patch) contributes the following:
1. It fixes an issue where the context could become empty.
2. It adds a test that ensures that - when using the global context - the active context is accurately updated

As a general style note - and something I didn't fix/change - we should be consistent in our instantiations of classes. Right now we veer back and forth between
$butler = new \Drupal\Context\Context;
and
$butler = new \Drupal\Context\Context();

pounard’s picture

pounard
Primary language French
commented

Nice, thanks for the patches, I didn't get the chance to see your changes I will diff with my branch tomorow.

Crell’s picture

Crell commented
StatusFileSize
new 1297142-stack-static-removal.patch11.71 KB

I applied the small fix patch from #12 to the branch. Posting a proper diff now.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented 
+++ b/modules/simpletest/tests/context.test
@@ -341,16 +341,36 @@ class ContextMockTestCase extends ContextTestCases {
+    $stack = new \Drupal\Context\ContextStack;

Do we really have to repeat "Context" a prefix within the Context namespace? Why not simply \Drupal\Context\Stack ?

Crell’s picture

Crell commented
Status: Needs review » Needs work 
+++ b/includes/Drupal/Context/Context.php
@@ -205,24 +216,8 @@ class Context implements ContextInterface {
   public function addLayer() {
-    $layer = new self(spl_object_hash($this));
+    $layer = new self($this);
+    $layer->setParent($this);
     return $layer;
   }

self has no constructor, so passing $this to it seems redundant. Either do that or do setParent().

+++ b/includes/Drupal/Context/Tracker.php
@@ -12,28 +12,39 @@ class Tracker {
   public function __destruct() {
     if (isset($this->context)) {
-      $this->context->__destruct();
+      $stack = $this->context->getStack();
+      if (isset($stack)) {
+        $stack->pop($this->context);
+      }
     }
   }

Since we can safely assume that a context object's stack won't change from the constructor to the destructor, why re-derive the stack? Just save a reference in the constructor and then reuse it in the destructor. Or would that create a circular reference? (Which is not as big an issue now with PHP 5.3's new garbage collection.)

+++ b/includes/bootstrap.inc
@@ -2421,8 +2426,18 @@ function _drupal_bootstrap_page_header() {
+function drupal_get_context(\Drupal\Context\ContextStack $new_stack = NULL) {
+  // This static property is infamous, but it's better here than in the OOP
+  // code, at least it does not taints the API.
+  static $stack;
+
+  if ($new_stack) {
+    $stack = $new_stack;
+  }
+
+  if (isset($stack)) {
+    return $stack->getActiveContext();
+  }
 }

I know Drupal does this sort of collector function global-but-not stuff a lot, but that's one of the things we're trying to eliminate. Is there no other way to make this work? Really?

I'm concerned about that static giving us trouble, too. It's not permanent since you can pass in a new stack, but still...

Overall I think this is reasonable, given the caveats above. I have to wonder, though, if at this point we should be considering using http://us2.php.net/manual/en/class.splstack.php instead of writing our own stack object. (That may not be viable because of the slicing we have to do when removing an item, but it's worth investigating I think.) Maybe subclassing it?

pounard’s picture

pounard
Primary language French
commented

True that the constructor should not use $this as parameter, I think this is a forgotten artefact of legacy code that has been forgotten.

About the Tracker destructor, yes, it was only to avoid storing another reference. But indeed, the correct method would be to store the stack into the Tracker directly to avoid problems if the stack is being changed into the ContextInterface object: I guess that it shouldn't happen, but as soon as modules will use it, it may be used in ways we can't predict (right now it cannot since the context interface signature specifies that once locked properties cannot be changed).

I re-introduced the static function variable in order to be able to fully remove any static properties in the OOP code. This is sad to have this back, but this is way better than having it in OOP code. If we were writing a Zend or Symfony based software, we would have an Application or HttpKernel or whatever object that would carry the original reference to this application-level stack (without any static or global state at all): because Drupal still have the procedural wrappers and do not have this kind of Application object, I suppose we have to stick with the static reference instead. I assume that while we don't have an Application object, procedural accessors are the materialization of the Application object which makes them the right place to carry this reference.

While reading the SplStack documentation I see that it carries a lot more features that we need right now: the side effect is by wanting to optimize the full stack using Spl based code, we might end-up by obfuscating it. I agree using Spl is good but IMHO only when we really need it. I don't see a feature in SplStack to remove all items from one in particular which would force us to iterate over it and remove all items one by one instead of using simply to array_*() functions. I think that right now, the PHP code stack remains the most efficient way. It's something that would need some more discussions.

Crell’s picture

Crell commented

Can you reroll the patch with the changes noted above then? (Sans SplStack for now.)

pounard’s picture

pounard
Primary language French
commented

Yes, but I can't do it before two days. Will do ASAP then.

Crell’s picture

Crell commented
Status: Needs work » Fixed

It turns out this issue was a blocker for #1262014: Move request path handling into context, because, guess what?, with a static stack we cannot have separate context object in our unit tests than we do in the host Drupal that is running the tests. Fail!

I therefore went ahead and made the changes discussed in #15 and #16, and merged this branch into WSCCI. Now off to path handling!

Thanks, pounard! I agree, this is cleaner than the internal static stack.

pounard’s picture

pounard
Primary language French
commented

Nice to see that you finally saw it could prove itself useful in the future, I just didn't guess it'd be that fast.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

(not verified) commented
Issue summary: View changes

Better description.