I am a little surprised the addLayer only allows new self and no way to spawn a different class.

CommentFileSizeAuthor
add_different_classes_to_layers.diff1.03 KBchx

Comments

Crell’s picture

Crell commented

That makes sense, I think. Should we pass in a class name, though, or an actual object? (And then call ->setParent() on that object, which if we type check against the interface we can guarantee it will have anyway.)

Stalski’s picture

Stalski commented

We need to be sure the object passed is a instance of DrupalContextInterface so it's must be the object, no?

Edit: But I like the ability of layering different classes.

Crell’s picture

Crell commented

If we wanted to enforce it in the signature, yes, it would have to be an object.

chx’s picture

chx commented

Yeah for that reason smthing like

function setLayer(Context $newContext = NULL) {
  if (!isset($newContext)) {
    $newContext = new self;
  }
  $newContext->setParent($this);
  return $newContext;
}

at which point , of course, the question is, do we even need this? Just create a new Context derivative instance and setParent and begone?

Crell’s picture

Crell commented

No, I was thinking more like:

function addLayer(ContextInterface $new_context = NULL) {
  if (!isset($new_context)) {
    $new_context = new self();
  }
  $new_context->setParent($this);
  return $new_context;
}

So in typical usage you'd just call addLayer() as you do now and there is no API change at all. But you could pass in anything that implements the right interface instead if you had some reason to.

Stalski’s picture

Stalski commented

Agreed.

chx’s picture

chx commented

Heh, #4 and #5 is the exact same code aside from the Context vs ContextInterface which is of course what I meant :) But the question remains. If I have a new SuperContext class then

$new_context = new SuperContext;
$new_context->setParent(drupal_get_context());

vs

$new_context = new SuperContext;
$new_context = drupal_get_context()->setLayer($new_context);
pounard’s picture

pounard
Primary language French
commented

I guess both are OK, as long as the addLayer function doesn't do more than it does now. Calling the addLayer() still is best.

Crell’s picture

Crell commented

I think I would somewhat rather option 2 from #7, as it makes it clear you're adding a new layer on top of a specific object. It also fits the degenerate case better, which is "whatever class you are, that's cool, just add another of yourself". With option 1, you always have to specify the class to use, which means lots more hard-coding.

I also think addLayer() is more descriptive than setLayer(), since you're adding to the stack/linked-list, not simply changing a value. It will change itself back at the appropriate time, by design.

Crell’s picture

Crell commented
chx’s picture

chx commented

Oh of course i meant addLayer, that was a typo from setParent. I will roll a patch.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Project: WSCCI » Drupal core
Version: » 8.x-dev
Component: Code » wscci

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

Crell’s picture

Crell commented
Status: Active » Closed (won't fix)

No longer relevant.