Problem/Motivation

It should be possible to initialize an entity browser with an existing selection. There are a few issues because the selection plugin is not necessarily bound to a specific url structure and its up to the selection plugin how the existing selection is presented to the user. Some selection widgets could handle multiple items (e.g. views) but some plugins are restricted so a single entity (e.g. entity_form)

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

webflo created an issue. See original summary.

webflo’s picture

FileSize
8.7 KB
webflo’s picture

webflo’s picture

FileSize
8.72 KB
webflo’s picture

Issue tags: +SprintWeekendBerlin
slashrsm’s picture

Status: Active » Needs work
Issue tags: +Media Initiative, +D8Media

Few comments:

  1. +++ b/modules/entity_form/src/Plugin/EntityBrowser/Widget/EntityForm.php
    @@ -40,13 +40,17 @@ class EntityForm extends WidgetBase {
    +    $entity = $form_state->get(['entity_browser', 'selected_entities', 0]);
    +    $op = isset($entity) ? 'edit' : 'add';
    +
    

    If there are more entities in the current selection we always edit just the first one. Would be nice to figure out how to support editing any of them.

    Recently we added "multi step" selection display, which allows reordering and removing of entities in the current selection. Maybe we could incorporated edit in there too.

  2. +++ b/modules/entity_form/src/Plugin/EntityBrowser/Widget/EntityForm.php
    @@ -40,13 +40,17 @@ class EntityForm extends WidgetBase {
           'inline_entity_form' => [
             '#type' => 'inline_entity_form',
    -        '#op' => 'add',
    +        '#op' => $op,
             '#handle_submit' => FALSE,
             '#entity_type' => $this->configuration['entity_type'],
             '#bundle' => $this->configuration['bundle'],
    +        '#entity' => $entity
    

    Didn't manage to really test this to find out by myself? How does edit functionality related to ordering of current selection. Selected entities are generally appended to the end of the list? Will the same happen in the edit case? Will this cause duplicated entities appearing in the list?

  3. +++ b/src/EntityCollection.php
    @@ -0,0 +1,37 @@
    +
    +class EntityCollection {
    +
    +  protected $entities;
    +
    +  /**
    +   * EntityCollection constructor.
    +   *
    +   * @param EntityInterface[] $entities
    +   */
    +  public function __construct($entities = []) {
    +    $this->entities = $entities;
    +  }
    +
    +  public function addEntity(EntityInterface $entity) {
    +    $this->entities[] = $entity;
    +  }
    +
    +  public function getEntities() {
    +    return $this->entities;
    +  }
    +
    +  public function isEmpty() {
    +    return (bool) !$this->entities;
    +  }
    +
    +}
    

    Doc blocks missing.

  4. +++ b/src/Form/EntityBrowserForm.php
    @@ -51,6 +76,12 @@ class EntityBrowserForm extends FormBase implements EntityBrowserFormInterface {
    +
    +    // Initialize select from query parameter
    +    $selection_uuid = $this->getRequest()->query->get('selection_uuid');
    +    if (!empty($selection_uuid) && $selection = $this->selectionStorage->get($selection_uuid)) {
    +      $form_state->set(['entity_browser', 'selected_entities'], $selection->getEntities());
    +    }
    

    Would it make sense to incorporate some kind of hash into this (for security reasons)?

  5. +++ b/src/Plugin/EntityBrowser/Display/IFrame.php
    @@ -126,7 +139,7 @@ class IFrame extends DisplayBase implements DisplayRouterInterface {
    -  public function displayEntityBrowser() {
    +  public function displayEntityBrowser(EntityCollection $selection = null) {
    

    This might break some other stuff (field widgets that @berdir created for file entity). Not a deal-breaker, but more of a note.

    Also, https://github.com/drupal-media/entity_browser/pull/140 might be related.

  6. +++ b/src/Plugin/EntityBrowser/Display/IFrame.php
    @@ -136,6 +149,12 @@ class IFrame extends DisplayBase implements DisplayRouterInterface {
    +
    +    if ($selection && !$selection->isEmpty()) {
    +      $selection_uuid = \Drupal::service('uuid')->generate();
    +      $this->selectionStorage->set($selection_uuid, $selection);
    +    }
    +
    

    We will need to add this to the Modal too.

    Would be nice to inject uuid if continue going forward with this approach.

killua99’s picture

This patch would work on the current dev code? I'll like to try it out.

webflo’s picture

FileSize
9.37 KB
slashrsm’s picture

This is definitely a feature that we want to have (and was planned since the beginning). Main problem that I have with the current approach is the fact that it is coupled to one specific widget type. I think we should architect this in a more general way. I also think we should split this into subtasks. This way it will be easier to wrap our heads around the problem and solve each individual problem separately. Divide and conquer! :)

Step 1: Allow passing existing selection on init

This is mostly done in #8. While there are other way to do it temp storage seems to be good approach. I was thinking about adding a configuration variable which would allow is to enable/disable this functionality.

We will also need to update selection propagation at the end of EB flow. Currently we append selected entities to the entities that might already exist on a parent form. That won't make sense any more when we implement this. I thought we could keep the current approach when there are no existing entities and replace selection entirely when there are.

Step 2: Allow passing existing selection from field widget
Depends on: step 1

We will need to update entity reference field widget to be able to propagate existing selection to the EB. We would make this configurable.

Step 3: Selection displays
Depends on: step 1

Selection displays need to handle existing selection. Multi step should work out of the box as it will read form state and find entities there. With "No display" there are few things we have to think about.

No display won't display existing entities by design. User won't have any idea which entities are already selected and will also have no way to do anything with them. That made me think if we should allow selection display plugins to declare whether they can accept initial selection or not (we could do this on annotation level).

If we go down that route we need to decide what to do if a selection display that can't handle this is used and entities are propagated anyway. Ignore them? Ignore them and display a message? Throw exception? I'd prefer loud and clear exception. This way we'd let people know that their configuration isn't right and they should fix it. This will also solve situation where entities are propagated while this feature is disable in configuration (Step 1).

Step 4: Editing entities
Depends on: steps 1 and 3

At this point we're able to initialize EB with existing entities. We're also able to reorder/add/remove them using multi step display. We should also able to edit them (second part of patch #8). This should be architected in a way that will allow any widget to be used as edit form. Of course, in some cases it does not make sense to allow that (how can upload tool edit file entities?). We should allow widget plugins to declare if they are able to edit for that reason (again, annotation).

Selection displays would be responsible to decide if we're editing and which entity should be edited. Multi step plugin could have "Edit" button for every displayed entity and clicking on it would trigger edit for it.

We will also need to decide which widget needs to be used for edit. We could take different approaches here (widget that is currently displayed, globally configured widget for edit, some kind of negotiation mechanism, ...) This should be up to the widget selector plugin and it's implementation.

Entity embed

I see two possible approaches for entity embed.

1. When embed is edited and entity browser is re-opened we select new entity and completely ignore existing one.
2. When entity browser is re-opened we automatically display edit of the embedded entity.

Case 1. is already possible and will be possible after this lands. Case 2. will need specific entity embed selection display plugin to handle specific business logic. While this might should cumbersome I think it is OK. We created this plugin types to allow this kind of fine tuning of the experience in the first place.

How does this sound? I am happy to start with step 1 (based on relevant hunks from #8).

webflo’s picture

FileSize
8.83 KB

Sorry no progress on proposal from #9. Just a re-roll

webflo’s picture

FileSize
7.76 KB
webflo’s picture

Status: Needs work » Needs review
webflo’s picture

FileSize
8.57 KB

The last submitted patch, 11: 2656196-11.patch, failed testing.

The last submitted patch, 11: 2656196-11.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 13: 2656196-12.patch, failed testing.

The last submitted patch, 13: 2656196-12.patch, failed testing.

chr.fritsch’s picture

Status: Needs work » Needs review
FileSize
16.29 KB

A little bit more WIP

  • Its saving the current selection in the store now after closing the browser
  • EntityCollection is now initialized with the field selection

Status: Needs review » Needs work

The last submitted patch, 18: initialize_entity-2656196-18.patch, failed testing.

The last submitted patch, 18: initialize_entity-2656196-18.patch, failed testing.

slashrsm’s picture

Status: Needs work » Needs review
FileSize
16.32 KB

Reroll.

Status: Needs review » Needs work

The last submitted patch, 21: 2656196_21.patch, failed testing.

The last submitted patch, 21: 2656196_21.patch, failed testing.

slashrsm’s picture

Status: Needs work » Needs review
FileSize
20.86 KB
18.35 KB

Takes step by step approach that I proposed in #9. It implements first step from that list, slightly changes approach in few areas, fixes test fails, adds test coverage.

phenaproxima’s picture

Status: Needs review » Needs work
  1. +++ b/entity_browser.services.yml
    @@ -22,3 +22,7 @@ services:
    +    class: Drupal\Core\KeyValueStore\KeyValueStoreExpirableInterface
    

    Um...I don't think the class key in a service definition can be an interface, because interfaces cannot be instantiated :) Yet the test gets this service, and apparently doesn't break. What is this devilry?

  2. +++ b/src/DisplayInterface.php
    @@ -36,11 +36,13 @@ interface DisplayInterface extends PluginInspectionInterface, ConfigurablePlugin
    +   * @param \Drupal\Core\Entity\EntityInterface[] $entities
    +   *   Existing selection that should be passed to the entity browser.
    

    Should be marked (optional).

  3. +++ b/src/Form/EntityBrowserForm.php
    @@ -30,13 +32,22 @@ class EntityBrowserForm extends FormBase implements EntityBrowserFormInterface {
    +    $this->selectionStorage = $selection_storage;
    +
    

    Nit: There's an extra line of whitespace.

slashrsm’s picture

Status: Needs work » Needs review
FileSize
20.93 KB
3.12 KB

Fixed #25.2, #25.3 and made sure key-value entry will expire after reasonable time.

WRT #25.1... If I remove it Symfony complains:

Symfony\Component\DependencyInjection\Exception\RuntimeException: Please add the class to service "entity_browser.selection_storage" even if it is constructed by a factory since we might need to add method calls based on compile-time checks.

So something needs to be set. I tried to change it to a complete nonsense (non-existing class) and it still worked. Go figure.... It seems that the interface is actually the best option.

slashrsm’s picture

FileSize
21.14 KB
639 bytes

Explaining the class value in the service definition.

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

Works for me.

  • slashrsm committed 0452e79 on 8.x-1.x authored by webflo
    Issue #2656196 by webflo, slashrsm, chr.fritsch, phenaproxima: Allow...
slashrsm’s picture

Status: Reviewed & tested by the community » Needs work

Committed. This covers #9.1. Let's continue with the steps that follow.

slashrsm’s picture

Status: Fixed » Closed (fixed)

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

Gábor Hojtsy’s picture

Issue tags: -Media Initiative