Implement static contexts

First attempt at this...

Currently displayed directly on the page manager edit form, should probably move to a dialog similar to other stuff.

Currently limited to entities, with autocomplete, then it stores the UUID and then exposes it as a special context class that loads the entity when accessed.

Not possible to remove context again...

1: static-context-2284005-1.patch queued for re-testing.

Didn't test the schema...

...

In Drupal 7, the static contexts are stored as part of the variant, thus each variant can have a different set of static contexts/relationships. I'm not sure, but from this patch it looks like the config is being stored at the page level.

Re-rolled with the new context definition objects.

@rlmumford: This is a very early proof of concept, I'm fine with moving this to displays, just keeping it up to date to get some initial feedback and looking for input on how to implement it better.. not sure how extendable this should/needs to be.

Re-roll.

Another re-roll.

Minor reroll as FormStateInterface cannot be accessed like an array. Patch and interdiff attached.

Thanks. Patch is much smaller, I think you lost the new files in the re-roll.

+++ b/src/Form/PageEditForm.php
@@ -304,7 +304,8 @@ class PageEditForm extends PageFormBase {
-    $input = $form_state['values']['static']['selection'];
+    $values = $form_state->getValues();
+    $input = $values['static']['selection'];

I would do $input = $form_state->getValue(array('static', 'selection') here.

And directly access it from $form_state below as well.

Or $static_values = getValue('static') and then get all the values from there.

Rerolled (with missing files) as per suggestions from #15.

From a site builders perspective this looks pretty good.

I was able to add a static context based on a node:

You can then use it for placing content on the block page, e.g.:
- Entity view (Content)
- Entity view (User)

Also if you have multiple contexts of the same type available, you get to select which context to choose from:

This seems to work on the whole except the autocomplete controller doesn't work with users as

$entity_type->getKey('label')

is an empty string. That means the autocomplete throws a fatal error.

Is there a generic entity autocomplete we can use in drupal core?

I stole the autocomplete from dynamic entity reference, which has adapted the entity reference selection plugins now, which probably fixed this for them. That doesn't really work for us, though, I think.

There is no generic autocomplete, a thing we could try is to special case users. Or consider to add a label key to users even if they also have a label callback.

Looked into this further, there is no core autocomplete and the User entity type does not provide any keys that we can search against. I guess this leaves a couple of options:

1. If $entity_type->getKey('label') is don't execute query and return an empty list.
2. If $entity_type->getKey('label') is empty exclude that entity type from the list of available contexts
3. If $entity_type->getKey('label') is empty then don't use autocomplete for that entity and instead tell the end user to type the id in
4. Add a custom work around for users to make the autocomplete work.

Hi Berdir,

Only just saw your reply. That makes sense, I'm not very keen on adding a work around for Users if we don't allow future module developers to provide a similar work around for their entity types.

Here's a potential way round it. I don't know what you think, it allows us to introduce a special case for users without us embedding it somewhere completely out of reach.

I'm pretty sure that test failure is a failure that is already in the code.

  /**
   * Tests changing the admin theme of a page.
   */
  protected function doTestAdminPath() {
    \Drupal::config('system.theme')->set('default', 'bartik')->save();
    $this->drupalGet('admin/foo');

I'm pretty sure you're not allowed to just set the admin theme anymore. I'm going to set this back to needs review and we should probably open another issue up for that bug.

See #2409321: Update Page Manager Tests for new Immutable Config.

The problem outlined in #20 can probably now be resolved using the EntityAutocomplete service introduced in #1959806: Provide a generic 'entity_autocomplete' Form API element

It is a bit more complicated due to the dynamic entity type, but I think dynamic_entity_reference solved this issue as well, so I'll look how they did it.

Here's a patch. I've updated it to use the entity.autocomplete_matcher service, which removes the need for the hacky stuff added in the previous patch.

@tim.plunkett I believe I've addressed all your concerns. For (9) I just removed the function, I'm not sure whether the form needs the validation handler as if the entity selected doesn't work it just does nothing.

General Question: Do we want to move the 'Add Static Context' stuff into a dialog and move the form into a method on the EntityLazyLoadContext class so that different plugins can be selected?

I didn't check this yet, but I think there's a lot more that we can simplify with the autocomplete now that core has the entity_reference field type. I stole a lot of the code from dynamic_entity_reference initially, so we should look at what they did: https://www.drupal.org/project/issues/dynamic_entity_reference, specifically this issue: https://www.drupal.org/node/2411981. Which, as you can see, no longer needs a custom autocomplete controller.

Alternatively, we could also just add some #ajax on the entity type selection and then replace the autocomplete widget with an ajax callback. The we don't need custom JS at all I think.

General Question: Do we want to move the 'Add Static Context' stuff into a dialog and move the form into a method on the EntityLazyLoadContext class so that different plugins can be selected?

+1 on moving it to a modal, it uses a lot of space right now.

I don't really get the second part, or how you want to do it. We certainly need to allow adding other things as well, and make that pluggable somehow, but I don't think adding something to EntityLazyLoadContext would help, as that is already entity specific.

I guess a proper UI/workflow would look like this:

1. An [Add context] button that opens a modal
2. That allows to select you what kind of context, this needs to be pluggable/extendable somehow. Right now, it would just be Entity.
3. After selecting that, then you get the current form with type and autocomplete selection

I'm not sure how much we want to do in this issue for 2. Defining an API/Event/plugins for this might delay this quite a bit, the best option might be to simply hardcode the form for the moment, and refactor it in a follow-up issue.

Attached is a patch that puts the Add Static context form into a dialog as well as adding "Edit" and "Delete" operations.

I've also added methods to the Page class for CRUD on static contexts. I'm not very happy with these, but they work for now and can be made to use a PluginCollection if/when we make the Static Contexts bit pluggable.

I don't really get the second part, or how you want to do it. We certainly need to allow adding other things as well, and make that pluggable somehow, but I don't think adding something to EntityLazyLoadContext would help, as that is already entity specific.

I meant that we the EntityLazyLoadContext would become an example of a "ConfigurableContext" Plugin. Is that clearer?

Defining an API/Event/plugins for this might delay this quite a bit, the best option might be to simply hardcode the form for the moment, and refactor it in a follow-up issue.

I agree with this. We can make the plugin in another issue. The plugin probably want's to be provided by another module/core anyway? Configurable contexts are going to be used by other modules (I'm thinking mini-panels) that don't depend on Page Manager at the moment.

1. +++ b/page_manager.routing.yml
   @@ -181,3 +207,10 @@ page_manager.selection_condition_delete:
   +
   +page_manager.autocomplete_entity:
   +  path: '/page_manager/autocomplete/{entity_type_id}'
   +  defaults:
   +    _controller: '\Drupal\page_manager\Controller\AutocompleteController::autocompleteEntity'
   +  requirements:
   +    _permission: 'administer pages'
   

   goal is still to get rid of this I think :)

2. +++ b/src/Entity/Page.php
   @@ -278,6 +288,62 @@ class Page extends ConfigEntityBase implements PageInterface {
   +    $static_contexts = $this->getStaticContexts();
   +    foreach ($static_contexts as $key => $static_context) {
   +      if ($static_context['name'] == $name) {
   +        $static_contexts[$key] = $configuration;
   +      }
   +    }
   +    $this->set('static_context', $static_contexts);
   

   That's a lot of code (and methods) just to avoid looking at $this->static_context directly, IMHO, that's not worth it?

3. +++ b/src/EventSubscriber/StaticContext.php
   @@ -0,0 +1,52 @@
   +    $static_contexts = $executable->getPage()->get('static_context');
   

   and it's not yet used here.

4. +++ b/src/Form/PageEditForm.php
   @@ -204,4 +309,52 @@ class PageEditForm extends PageFormBase {
   +  /**
   +   * Form submit callback to add a context reference.
   +   */
   +  public function submitStaticContext(array $form, FormStateInterface $form_state) {
   +    $input = $form_state->getValue(['static', 'selection']);
   +
   +    // Take "label (entity id)', match the ID from parenthesis when it's a
   +    // number.
   +    if (preg_match("/.+\((\d+)\)/", $input, $matches)) {
   +      $match = $matches[1];
   +    }
   +    // Match the ID when it's a string (e.g. for config entity types).
   

   this shouldn't be needed anymore unless we still call it from the new methods, but I guess not?

I'll see what I can do about those missing tests :)

Tests added to test the static context.
Some bugs found and fixed.

1. +++ b/src/Form/StaticContextFormBase.php
   @@ -153,6 +153,12 @@ abstract class StaticContextFormBase extends FormBase {
   +      $form_state->setError($form, t('Invalid entity type %entity_type', array('%entity_type' => $input)));
   

   Invalid entity, not entity type.

   Also, bonus points for something like @entity_type_label, based on $entity_type->getLabel()

2. +++ b/src/Tests/PageManagerAdminTest.php
   @@ -345,6 +346,64 @@ class PageManagerAdminTest extends WebTestBase {
   +    $this->assertText(t('Entity view (User)'));
   +    $this->assertText(t('Member for'));
   

   As discussed, add a comment here, that viewing a user does not not actually display anything that we can identify as belonging to the right user. But just by showing that the block is here and is being rendered should be enough to test that the context is working.

3. +++ b/src/Tests/PageManagerAdminTest.php
   @@ -345,6 +346,64 @@ class PageManagerAdminTest extends WebTestBase {
   +      'context[name]' => 'test_context',
   +      'context[label]' => 'Test context edited',
   +      'context[entity_type]' => 'user',
   +      'context[selection]' => 'administrator ' . '(' . $this->loggedInUser->id() . ')',
   

   I'm not sure if the name should be editable and what happens if you do that.

Entity type label added instead of 'entity'. comment added in the test as suggested.
the name also edited and asserted.

Fixed some wording and removed trailing whitespace from patch in #38.

As discussed with Berdir,
Custom autocorrect plugins dropped and the entity_autocomplete form element used.
formValidation dropped from StaticContextFormBase (no longer required)
Tests corrected accordingly and comment modification from #40

Messed up patch! Interdiff is fine

Hm.. I dropped a test line accidentally. added it again

1. +++ b/src/Form/PageEditForm.php
   @@ -19,6 +23,36 @@ use Drupal\Core\Url;
   +   * @param \Drupal\Core\Entity\EntityManagerInterface $entity_manager
   +   *   The entity manager.
   +   */
   +  public function __construct(QueryFactory $entity_query, EntityManagerInterface $entity_manager) {
   +    $this->entityQuery = $entity_query;
   +    $this->entityManager = $entity_manager;
   +  }
   ...
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public static function create(ContainerInterface $container) {
   +    return new static(
   +      $container->get('entity.query'),
   +      $container->get('entity.manager')
   +    );
   +  }
   

   Are these still used?

2. +++ b/src/Form/PageEditForm.php
   @@ -204,4 +309,39 @@ class PageEditForm extends PageFormBase {
   +  /**
   +   * Form submit callback to add a context reference.
   +   */
   +  public function submitStaticContext(array $form, FormStateInterface $form_state) {
   +    $input = $form_state->getValue(['static', 'selection']);
   +
   +    // Take "label (entity id)', match the ID from parenthesis when it's a
   +    // number.
   +    if (preg_match("/.+\((\d+)\)/", $input, $matches)) {
   +      $match = $matches[1];
   

   This is a left-over, can be removed.

3. +++ b/src/Form/StaticContextFormBase.php
   @@ -0,0 +1,213 @@
   +    $form['context'] = [
   +      '#type' => 'details',
   +      '#title' => $this->t('Add static context'),
   +      '#open' => TRUE,
   +      '#tree' => TRUE,
   +    ];
   +
   

   The details element here seems strange. It made sense when it was part of the whole page, but it doesn't server a purpose here. I'd drop this completely.

4. +++ b/src/Tests/PageManagerAdminTest.php
   @@ -38,7 +38,7 @@ class PageManagerAdminTest extends WebTestBase {
    
   -    $this->drupalLogin($this->drupalCreateUser(['administer pages', 'access administration pages', 'view the administration theme']));
   +    $this->drupalLogin($this->drupalCreateUser(['administer pages', 'access administration pages', 'view the administration theme']), 'administrator');
   

   I'm not sure what this is, drupalLogin() only has one argument?

Leftovers dropped. context form element dropped and changes made to the forms and tests accordingly. the second argument should be inside the createUser method :-)

#2375695-22: Condition plugins should provide cache contexts AND cacheability metadata needs to be exposed pointed out that it's not clear how to make this set the right cache contexts. Could you explain in a bit more detail the obstacles you're seeing?

Reroll to get an applicable patch.

Here's a re-roll of the patch which passes all tests.

Adding another patch that includes a basic test for the static context feature.

Removed the accidentally included patch file from the patch.

Just reviewing the interdiffs.

+++ b/src/Context/EntityLazyLoadContext.php
@@ -42,5 +42,14 @@ class EntityLazyLoadContext extends Context {
+    if (!$this->contextValue) {
+      $this->getContextValue();
+    }

Might be useful to add a comment here. Something like // Ensure that the entity is loaded before checking if it exists.

1. +++ b/src/Tests/StaticContextTest.php
   @@ -0,0 +1,101 @@
   +    // Create a node, and check its page.
   +    $node = $this->drupalCreateNode(['type' => 'article']);
   +    $this->drupalGet('node/' . $node->id());
   +    $this->assertResponse(200);
   +    $this->assertText($node->label());
   +    $this->assertTitle($node->label() . ' | Drupal');
   

   Not sure we need the asserts here.

   Let's also create a second node and add + display them both, so we can test with multiple static contexts.

2. +++ b/src/Tests/StaticContextTest.php
   @@ -0,0 +1,101 @@
   +    // Create a new page entity to take over node pages.
   +    $edit_page = [
   +      'label' => 'Static node context',
   

   We're not taking over anything anymore, so remove that part from the comment.

3. +++ b/src/Tests/StaticContextTest.php
   @@ -0,0 +1,101 @@
   +    $this->drupalPostForm('admin/structure/page_manager/add', $edit_page, 'Save');
   +    // Their pages should now use the default 404 display variant.
   +    $this->drupalGet('static-context');
   +    $this->assertResponse(404);
   +    $this->assertNoText($node->label());
   +
   +    //$this->drupalGet('static-context');
   +    // Add a static context to the page.
   +    $this->drupalGet('admin/structure/page_manager/manage/' . $edit_page['id']);
   

   The drupalGet() + asserts here can be removed I think. Then you're already on the right page and don't have to load gain.

Ok applied the changes as suggested.

Looks pretty good. Some feedback below.

1. +++ b/config/schema/page_manager.schema.yml
   @@ -29,6 +29,25 @@ page_manager.page.*:
   +    static_context:
   +      type: sequence
   +      label: Static context list
   +      sequence:
   +        - type: mapping
   +          label: 'Static context'
   +          mapping:
   +            machine_name:
   +              type: string
   +              label: 'Machine-readable name of the context'
   +            label:
   +              type: label
   

   We could also store this keyed by the machine name I guess. Not sure what's better, that would make some of the methods easier to implement and the machine name has to be unique anyway.

2. +++ b/page_manager.routing.yml
   @@ -82,6 +82,32 @@ page_manager.access_condition_delete:
    
   +#### Static Contexts
   +
   +page_manager.static_context_add:
   

   Does routing.yml have similar comments elsewhere? If not then I'm not sure we should keep it.

3. +++ b/src/Form/PageEditForm.php
   @@ -204,4 +307,24 @@ class PageEditForm extends PageFormBase {
   +  /**
   +   * Form submit callback to add a context reference.
   +   */
   +  public function submitStaticContext(array $form, FormStateInterface $form_state) {
   +    $input = $form_state->getValue(['static', 'selection']);
   +    $entity_type = $form_state->getValue(['static', 'entity_type']);
   +    $entity = $this->entityManager->getStorage($entity_type)->load($input);
   +    $new_static = [
   +      'machine_name' => $form_state->getValue(['static', 'machine_name']),
   +      'label' => $form_state->getValue(['static', 'label']),
   +      'type' => 'entity:' . $entity_type,
   +      'value' => $entity->uuid(),
   

   As commented before, This method is a left-over from the old code and can be removed.

4. +++ b/src/Form/StaticContextDeleteForm.php
   @@ -0,0 +1,80 @@
   +    return $this->t('Are you sure you want to delete the static context %name?', ['%name' => $this->staticContext['label']]);
   

   placeholder should use label, not name. Possibly elsewhere too.

   In case we change to key the contexts by name then this would have to just store the machine name and fetch it from the page if needed.

5. +++ b/src/Form/StaticContextFormBase.php
   @@ -0,0 +1,215 @@
   +      '#required' => FALSE,
   

   machine name is also required.

6. +++ b/src/Form/StaticContextFormBase.php
   @@ -0,0 +1,215 @@
   +    if ($form_state->hasValue(['entity_type'])) {
   +      $entity_type = $form_state->getValue(['entity_type']);
   

   We can remove the [] here.

7. +++ b/src/Tests/PageManagerAdminTest.php
   @@ -379,6 +380,64 @@ class PageManagerAdminTest extends WebTestBase {
   +   * Tests adding static context page.
   +   */
   +  protected function doTestStaticContext() {
   +    $this->drupalGet('admin/structure/page_manager/manage/foo');
   

   Looks like there were already some tests here, let's remove them, we don't need both. I prefer using nodes as they are easier to validate that they actually show up.

8. +++ b/src/Tests/StaticContextTest.php
   @@ -0,0 +1,122 @@
   +    // Open the page and verify that the node from the static context is there.
   +    $this->drupalGet($edit_page['path']);
   +    $this->assertText($node->label());
   +    $this->assertText($node->get('body')->getValue()[0]['value']);
   +    $this->assertText($node2->label());
   +    $this->assertText($node2->get('body')->getValue()[0]['value']);
   

   Just like the existing test, this should also testing editing and deleting static context. You could e.g. switch one context to use the other node and then you should see the same node twice on the page.

placeholder should use label, not name. Possibly elsewhere too.

I replaced this for the static context related output. But there are other parts of code which use this wrong pattern, for example it can be found in src/Form/PageDeleteForm.php:

return $this->t('Are you sure you want to delete the page %name?', ['%name' => $this->entity->label()]);

I applied the other suggested changes and combined the tests.

I think is finally close. A few more minor things below.

1. +++ b/page_manager.routing.yml
   @@ -82,8 +78,6 @@ page_manager.access_condition_delete:
    
   -#### Static Contexts
   -
    page_manager.static_context_add:
   

   I think I was unclear here :) What I was saying is we shouldn't add a document like this *unless* other places in that file alraedy do it. They clearly do, so we should keep them and re-add ours.

2. +++ b/src/Entity/Page.php
   @@ -341,11 +323,7 @@ class Page extends ConfigEntityBase implements PageInterface {
      public function removeStaticContext($name) {
        $static_contexts = $this->getStaticContexts();
   -    foreach ($static_contexts as $key => $static_context) {
   -      if ($static_context['machine_name'] == $name) {
   -        unset($static_contexts[$key]);
   -      }
   -    }
   +    unset($static_contexts[$name]);
        $this->set('static_context', $static_contexts);
        return $this;
   

   This can also be simplified to directly work on $this->static_context now.

3. +++ b/src/Form/StaticContextEditForm.php
   @@ -40,27 +39,14 @@ class StaticContextEditForm extends StaticContextFormBase {
   +    // The machine name mustn't be changed after a static context was created.
   +    $form['machine_name'] = array(
   +      '#type' => 'value',
   +      '#value' => $name,
   +    );
   

   I think this can be simplified a bit to:
   // The machine name of an existing context is read-only.

4. +++ b/src/Form/StaticContextFormBase.php
   @@ -92,12 +92,12 @@ abstract class StaticContextFormBase extends FormBase {
   -      '#default_value' => $this->staticContext['machine_name'] ?: '',
   +      '#default_value' => $name ? $name : '',
   

   $name is an empty string for the add form so you can just use $name I think?

5. +++ b/src/Tests/StaticContextTest.php
   @@ -117,6 +117,60 @@ class StaticContextTest extends WebTestBase {
        $this->assertText($node2->get('body')->getValue()[0]['value']);
   +
   +
   +
   +
   +    // Change the second static context to the first node.
   

   One empty line should be enough ere.

Here's another patch with the suggested changes.

I think I was unclear here :) What I was saying is we shouldn't add a document like this *unless* other places in that file alraedy do it

I agree, it was a bit unclear. I interpreted "other places" as other modules, for example core modules which don't use such comments, so i removed them. Anyway, re-added them now.

1. We can actually inject I think since we create those objects ourself.
6. That's a left-over from an old test where we used the user as context. Can be reverted.

Here's the new patch including the suggested changes/improvements

I think this addresses #70, time to get it in? :)

That seems to be a core issue: #2536456: Autocomplete is broken (its JS is not loaded). So they're probably broken everywhere, not just here.

Changes look good, missed the missing injection in StaticContexts.