Problem/Motivation

Partially depends on #2284005: Implement static contexts

D7 Page Manager has the ability to add contexts based on relationships:
Node context -> node author -> User Context

Proposed resolution

Write the API/storage for arbitrary contexts
Write an event subscriber to get them into the list of contexts
Write a UI to control them for a given page.

Remaining tasks

See resolution

User interface changes

An added UI for context relationships

API changes

N/A

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir’s picture

Assigned: Unassigned » Berdir

Working a bit on this...

Berdir’s picture

Status: Active » Needs review
FileSize
7.63 KB

Experimenting a bit with this, the patch adds a table with the current context and a select field to add more things that I'm extracting from the context, given those are entities..

Current problems/questions:
- Some hardcoded assumptions about entities and how they work, although some of the code could be refactored to be more generic, but not everything.
- Not sure what to save exactly, currently I just store a string that I can later on use to identify what it was originally.
- In the event listener, I will need a way to get the parent context and extract the value from there if it exists
- Not yet sure what to do with the label exactly, in the select I'm displaying it with the hierarchy, but would have to persist that somehow.

The code is super ugly, don't look too long or you might get blind ;)

Status: Needs review » Needs work

The last submitted patch, 2: 2284019-page-manager-references-2.patch, failed testing.

tim.plunkett’s picture

  1. +++ b/src/Form/PageEditForm.php
    @@ -9,7 +9,17 @@ namespace Drupal\page_manager\Form;
    +use Drupal\Core\Entity\TypedData\EntityDataDefinition;
    +use Drupal\Core\Entity\TypedData\EntityDataDefinitionInterface;
    +use Drupal\Core\Plugin\Context\Context;
    +use Drupal\Core\TypedData\ComplexDataDefinitionInterface;
    +use Drupal\Core\TypedData\ComplexDataInterface;
    +use Drupal\Core\TypedData\DataReferenceDefinitionInterface;
    +use Drupal\Core\TypedData\ListDataDefinition;
    +use Drupal\Core\TypedData\ListDataDefinitionInterface;
    

    Hmm, maybe this belongs in another class? Or maybe just another method...

  2. +++ b/src/Form/PageEditForm.php
    @@ -43,6 +53,79 @@ class PageEditForm extends PageFormBase {
    +    foreach ($contexts as $name => $context) {
    +      $context_definition = $context->getContextDefinition();
    +      $form['context']['available_context']['#rows'][] = array(
    +        $context_definition['label'],
    +        $name,
    +        $context_definition['type'],
    +      );
    +
    +      if (strpos($context_definition['type'], 'entity:') === 0) {
    +        $entity_data_definition = EntityDataDefinition::createFromDataType($context_definition['type']);
    +        // Add the bundle manually for entities that have no bundle key,
    +        // improve this...
    +        if (!\Drupal::entityManager()->getDefinition($entity_data_definition->getEntityTypeId())->hasKey('bundle')) {
    +          $entity_data_definition->setBundles(array($entity_data_definition->getEntityTypeId()));
    +        }
    +        foreach ($entity_data_definition->getPropertyDefinitions() as $field => $field_definition) {
    +          if ($field_definition instanceof ListDataDefinitionInterface) {
    +            $field_item_definition =$field_definition->getItemDefinition();
    +            if ($field_item_definition instanceof ComplexDataDefinitionInterface) {
    +              foreach ($field_item_definition->getPropertyDefinitions() as $property => $property_definition) {
    +                if ($property_definition instanceof DataReferenceDefinitionInterface) {
    +                  if ($property_definition->getTargetDefinition() instanceof EntityDataDefinitionInterface) {
    +                    $target_definition = $property_definition->getTargetDefinition();
    +                    // Ignore references that were already added.
    +                    if (isset($contexts[$name . '.' . $field . '.' . $property])) {
    +                      continue;
    +                    }
    +                    $label = String::format('@context_label > @field > @property (@type)', array(
    +                      '@context_label' => $context_definition['label'],
    +                      '@field' => $field_definition->getLabel(),
    +                      '@property' => $property_definition->getLabel(),
    +                      '@type' => $target_definition->getDataType(),
    +                    ));
    +                    $references[$name . '.' . $field . '.' . $property . '.' . $target_definition->getDataType()] = $label;
    +                  }
    +                }
    +              }
    +            }
    +          }
    +        }
    +      }
    

    https://twitter.com/MortimerGoro/status/476649081146994688/photo/1
    :D

rlmumford’s picture

This might already be possible but its something that I will need in drupal 8.

If if have a node and a user in the Context stack I want to be able to use a relationship to get the Membership entity that links those two together. I think this means that relationships need to be more than *just* entity properties as entity properties only depend on one entity.

Can I suggest we make a Context Relationship plugin type, that can declare what entity types it depends on and then return an entity when given those dependencies. We can then make a single 'EntityProperty' relationship plugin and make loads of derivatives.

tim.plunkett’s picture

  1. +++ b/src/EventSubscriber/ReferencesContext.php
    @@ -0,0 +1,55 @@
    +    $executable = $event->getPageExecutable();
    +    $references = $executable->getPage()->get('references');
    

    It's $event->getPage() now

  2. +++ b/src/Form/PageEditForm.php
    @@ -43,6 +53,79 @@ class PageEditForm extends PageFormBase {
    +      if (strpos($context_definition['type'], 'entity:') === 0) {
    +        $entity_data_definition = EntityDataDefinition::createFromDataType($context_definition['type']);
    +        // Add the bundle manually for entities that have no bundle key,
    +        // improve this...
    +        if (!\Drupal::entityManager()->getDefinition($entity_data_definition->getEntityTypeId())->hasKey('bundle')) {
    +          $entity_data_definition->setBundles(array($entity_data_definition->getEntityTypeId()));
    +        }
    +        foreach ($entity_data_definition->getPropertyDefinitions() as $field => $field_definition) {
    +          if ($field_definition instanceof ListDataDefinitionInterface) {
    +            $field_item_definition =$field_definition->getItemDefinition();
    +            if ($field_item_definition instanceof ComplexDataDefinitionInterface) {
    +              foreach ($field_item_definition->getPropertyDefinitions() as $property => $property_definition) {
    +                if ($property_definition instanceof DataReferenceDefinitionInterface) {
    +                  if ($property_definition->getTargetDefinition() instanceof EntityDataDefinitionInterface) {
    +                    $target_definition = $property_definition->getTargetDefinition();
    +                    // Ignore references that were already added.
    +                    if (isset($contexts[$name . '.' . $field . '.' . $property])) {
    +                      continue;
    +                    }
    +                    $label = String::format('@context_label > @field > @property (@type)', array(
    +                      '@context_label' => $context_definition['label'],
    +                      '@field' => $field_definition->getLabel(),
    +                      '@property' => $property_definition->getLabel(),
    +                      '@type' => $target_definition->getDataType(),
    +                    ));
    +                    $references[$name . '.' . $field . '.' . $property . '.' . $target_definition->getDataType()] = $label;
    +                  }
    +                }
    +              }
    +            }
    +          }
    +        }
    +      }
    

    That's a lot of }
    Can this be split out to another class/service, or at least a protected method?

  3. +++ b/src/Form/PageEditForm.php
    @@ -217,6 +300,16 @@ class PageEditForm extends PageFormBase {
    +    $references = (array) $this->entity->get('references');
    ...
    +    $this->entity->set('references', $references);
    

    Is this worth a getter/setter?

dsnopek’s picture

Now that contexts are on the variant rather than page, should relationships also be on the variant?

Putting them on the page means we can't get a relationship from a static context, which would be a rare use case, but I could see it. For example, if we have a static context pointing at a node which has a entity reference on it, and we want to use data from the referenced entity, which could get updated on the node at any time. (The alternative would be two static contexts to both the node, and the referenced entity, but then you have to edit the variant to change the referenced entity rather than just editing the node.)

dsnopek’s picture

I just checked in page_manager in Drupal 7, and it's possible to have relationships based on static contexts, and the relationship information is stored on the page_manager_handlers table (ie. on the variant). So, for feature parity with D7, I think we should store the relationships on the variant in D8 as well!

dsnopek’s picture

Assigned: Berdir » dsnopek

The massive foreach/if/if/if/etc statement mentioned in #4.2 and #6.2 can be replaced with simply using the TypedDataResolver from CTools. There is also an existing relationship schema in CTools (ctools.relationship) which is also used by the D8 Pathauto fork, which we can leverage here.

So, I'm going to brew up a super quick patch that takes advantage of those things, and copies some of the code from the CTools relationship forms (which depend on the Wizard API) into some forms that fit in with the current page_manager UI!

My goal is just to get some of the scaffolding for this in place, and make sure (with timplunkett, EclipseGC and berdir) that this is a good direction. (Then it might make sense to actually hand off to Tim to finish and make high quality enough to get committed.)

dsnopek’s picture

Assigned: dsnopek » Unassigned
Status: Needs work » Needs review
Issue tags: +Needs tests
FileSize
22.21 KB

Here's a first pass at this! It works in my manual testing, but has no automated tests yet.

Status: Needs review » Needs work

The last submitted patch, 10: page_manager-relationships-2284019-10.patch, failed testing.

dsnopek’s picture

Whoa! Not sure how I broke all the tests, but I'll need to look into that... :-) In any case, I'd love some high-level review on the approach!

dsnopek’s picture

Status: Needs work » Needs review
FileSize
24.78 KB
3.08 KB

This may or may not fix the test failures! It doesn't add any tests for new functionality, though, which we still need.

Status: Needs review » Needs work

The last submitted patch, 13: page_manager-relationships-2284019-13.patch, failed testing.

The last submitted patch, 13: page_manager-relationships-2284019-13.patch, failed testing.

dsnopek’s picture

Status: Needs work » Needs review
FileSize
25.21 KB
442 bytes

Random idea to try and fix the last failure. Didn't test locally - leaving the hard work to testbot. :-)

tim.plunkett’s picture

interdiff is from #13

The last submitted patch, 16: page_manager-relationships-2284019-16.patch, failed testing.

tim.plunkett’s picture

  1. +++ b/src/Entity/PageVariant.php
    @@ -257,7 +267,9 @@ public function getContexts() {
    +      $contexts = array_merge($static_contexts, $page_contexts);
    ...
    +      $this->contexts = array_merge($contexts, $relationships);
    

    I would write one array_merge here, so that we're explicit about the order. I believe the order @EclipseGC and I discussed was array_merge($relationships, $static_contexts, $page_contexts) since the last one wins.

  2. +++ b/src/Form/PageVariantEditForm.php
    @@ -305,6 +307,12 @@ protected function buildContextForm() {
    +    foreach ($page_variant->getRelationships() as $token => $relationship) {
    

    you can remove the $token bit

  3. +++ b/src/Form/PageVariantEditForm.php
    @@ -397,6 +495,9 @@ public function save(array $form, FormStateInterface $form_state) {
    +    // Unset 'relationships' because it doesn't hold the complete data.
    +    $form_state->unsetValue('relationships');
    

    Is this the best place to do this? Isn't there a copyValuesToWhatever method?

I need to apply it and look at the UI more, but that's what I got from reading the patch.

dsnopek’s picture

@tim.plunkett: Thanks!

#19.1: Ok! We still need the first array_merge() because we have to pass that into the $context_mapper->getRelationshipValues(), but having the 2nd array_merge() explicitly list all the "input contexts" definitely makes sense.

#19.2: Done!

#19.3: No, there is in PageEditForm, but not PageVariantEditForm. Added a copyFormValuesToEntity() and unset it there instead!

Status: Needs review » Needs work

The last submitted patch, 20: page_manager-relationships-2284019-20.patch, failed testing.

dsnopek’s picture

Status: Needs work » Needs review
FileSize
25.47 KB

Gah, sorry, my last patch didn't include the changes from #17. Here's a new patch, but the interdiff is still good.

rlmumford’s picture

Status: Needs review » Needs work

I'm going to raise the same question I raised two years ago, what about relationships that are not just entity properties? This patch looks like the only type of relationship it supports is a entity property (nested or otherwise) and I can't see anyway of creating plugins for relationships in here.

There are lots of use cases where you might want custom logic and/or relationships that depend on more than one context. E.g you have a node context and the currently logged in user context and you want a relationship that adds the OG membership that connects the two together.

Can we make relationships context aware plugins and provide an 'entity property' relationship plugin that only requires one context and works the same way as the code in the patch? I think that not being able to write relationship plugins would be a significant regression.

dsnopek’s picture

I'm going to raise the same question I raised two years ago, what about relationships that are not just entity properties? This patch looks like the only type of relationship it supports is a entity property (nested or otherwise) and I can't see anyway of creating plugins for relationships in here.

This patch leverages the TypedDataResolver service from CTools, so it can support anything that that service can support. It's true that currently the TypedDataResolver only supports entity properties, but there is a @todo in TypedDataResolver::getTokensFromComplexData():

      // Only expose references as tokens.
      // @todo Consider to expose primitive and non-reference typed data
      //   definitions too, like strings, integers and dates. The current UI
      //   will not scale to that.

So, it's definitely in the goals of TypedDataResolver to support any typed data. But that doesn't affect this patch - as soon as this is changed in CTools, it'll just start working in page_manager!

But that means you'd eventually create your own "plugins for relationships" by affecting the typed data definition of the thing you're interested in, and it should just work.

dsnopek’s picture

Status: Needs work » Needs review

Er, and still needs review. :-)

rlmumford’s picture

This patch leverages the TypedDataResolver service from CTools, so it can support anything that that service can support. It's true that currently the TypedDataResolver only supports entity properties, but there is a @todo in TypedDataResolver::getTokensFromComplexData():

The todo in that function has nothing to do with a relationship requiring more than one context - its talking about allowing relationships to primitive data types.

I find it very hard to see how a "relationship" system entirely based on tokens could ever do complex relationship. Sure the convertTokenToContext() method does get passed all of the contexts but the only thing it does is pass on to $this->getContextFromProperty() that only gets given the base context so you would not be able to do relationships based on 2 or more contexts.

getTokensForContexts() would need to somehow add magical tokens to a context based on the other contexts that were there. So, for example, if you had a node and user in contexts it would somehow expose a token on the node that included the user context name for the membership. It might be CTools plan to make typed data resolver do that but I can't see any evidence of it going that way yet.

dsnopek’s picture

Status: Needs review » Needs work

... so you would not be able to do relationships based on 2 or more contexts.

Ok, so what you want, is context-aware relationship plugins that can have more contexts mapped to them (similar to how block and other context aware plugins have contexts mapped) so that a single new context can be created by the relationship plugin based on multiple contexts?

Looking at relationship plugins in D7, this is exactly how they work and doing this is completely possible. In fact, your OG membership example was exactly right - it takes two required contexts:

http://cgit.drupalcode.org/og/tree/plugins/relationships/og_membership_f...

While we could implement a relationship like this with typed data in D8 for the current user, we couldn't use a different user from context, for example, the author of a node from context.

So, now I think I've switched sides, and I'm starting to agree with you. :-) This also makes me question how we did static contexts as well... Maybe static contexts and relationships could be the same plugin type, but relationships just happen to require additional contexts?

@EclipseGC / @tim.plunkett / @berdir: Thoughts?

tim.plunkett’s picture

+++ b/src/Form/PageVariantEditForm.php
@@ -383,6 +397,101 @@ class PageVariantEditForm extends PageVariantFormBase {
+  protected function copyFormValuesToEntity(EntityInterface $entity, array $form, FormStateInterface $form_state) {
+    // Unset 'relationships' because it doesn't hold the complete data.
+    $form_state->unsetValue('relationships');
+
+    parent::copyFormValuesToEntity($entity, $form, $form_state);

In PageEditForm::copyFormValuesToEntity, we restore the $form_state after the values are copied to the entity. I think that should be done here too.

dsnopek’s picture

Status: Needs work » Needs review
Issue tags: +Needs UI
FileSize
26.06 KB

Ok, here's the beginning of a new patch that adds relationship plugins. It's barely even started - there's still loads to do, including: getting the plugin collection figured out right (I'm terrible with plugin collections), the accessors for relationship plugins, the config schema, UI to show the relationship configuration forms, and actually putting the relationship contexts into the available contexts.

So, yeah, lots. :-) But there's relationship plugins!

No interdiff because this is starting over.

tim.plunkett’s picture

fago’s picture

TypedDataResolver - we have a data fetcher service in Rules for now, which works generically on typed data and can work on typed data objects as well as definitions. It's all done, has tests and is usable, but we'll probably move it over to a separate typed data module for better re-usability.

Berdir’s picture

Ctools has such a class/service too, you reviewed that a bit in drupal camp vienna. What about getting it into 8.1 or maybe 8.2 instead of a module?

tim.plunkett’s picture

Issue tags: +beta blocker
Kristen Pol’s picture

Status: Needs review » Needs work

Patch from #30 needs a re-roll.

andypost’s picture

Some forms are moved to ctools so looks this patch should be split between both modules

DamienMcKenna’s picture

netw3rker’s picture

Any chance we can breathe some life into this one?

I tried to reroll #30, but the move to using the ctools Wizard API #2550879: Use CTools Wizard API to add/edit Pages (and move plugin UI using PluginWizardInterface) has made rerolling this a bit too complex for me to resolve myself.

Also, even if there is a re-roll of this, it looks like #2511568: Create "context stack" service where available contexts can be registered might need to happen first in order for the TODO's to be addressed. but I'm not exactly sure on that one.

FWIW, To me, this is a relatively important part of context resolution when dealing with Views arguments. for example, when creating a View with blocks that have a context filter and a more link.

lets say you have two content types: a primary, and a secondary that contains an entityreference back to the primary. If you want to compose a view block that shows all the related information about the primary, even if you are looking at any of the secondary items, the only option is to create two blocks in the view. One block will take the ID of the primary content type, and list related items. the other must take the id of the secondary, add a relationship to the primary, then show the information. If you chose to use the 'more link', then you must create two page displays because you'll need the contextual ID of either the primary or the secondary depending on what you are looking at.

Using page manager/panels, adding a relationship solves this problem. I can add variant1 for the primary content type, and variant 2 for the secondary content type. then in the secondary content type i can add a relationship to the primary. Now I only need to configure one "related items" block in views, and can pass in the correct context entity depending on the variant. This then allows me to have just one more-link destination, and all is right with the world.

MustangGB’s picture

I too had a shot at re-rolling this, also without much luck.

I guess not a good first candidate to tackle after not having touched D8 code for a year or so.

DamienMcKenna’s picture