Remove ctools dependency by building own admin UI

Regarding point #2 -- absolutely agree. Can we do a thing where we check if the plugins are implementing ConfigurablePluginInterface, and skip their config steps if they aren't? I believe the CTools Wizard API specifically includes provisions to do this, although I don't know exactly how to leverage them.

One concern I have is that EB uses an awful lot of jargon-ey words, which can potentially confuse the hell out of users. I feel that a good start would be to provide a #description for every single form field in the core EB configuration UI. We should also leverage the #states system as much as possible to hide fields which are not absolutely necessary.

I would suggest:

- drop the wizard UI
- have several forms with 2nd-level tabs to navigate between them (or everything in one form)
- use entity operations to provide links directly to the different tabs

I was pointed here by @marcoscano after creating #2962064: Alternative to wizard-like configuration screen?. I second the suggestion to drop the wizard UI.

I've started working on this.

This patch drops the wizard UI, and adds 2nd-level tabs to navigate between them.

I wanted to post it as a work in progress. There's a major bug where it works fine, as far as I can tell, for editing, but I'm getting an error when I try to create a new entity browser.
Drupal\Component\Plugin\Exception\PluginNotFoundException: The "" plugin does not exist. in Drupal\Core\Plugin\DefaultPluginManager->doGetDefinition() (line 52 of core/lib/Drupal/Component/Plugin/Discovery/DiscoveryTrait.php).

I'll keep working on it, but if someone wants to try out what I have so far and see if they can figure out what the bug is, that'd be great.

Also to do: "use entity operations to provide links directly to the different tabs".

Also, I think hook_menu_local_tasks_alter to hide the local tasks that don't have any configuration settings would be great.

Also, we'll probably need to update the tests.

I have added an updated patch that fixes the bug on entity_browser creation.

Next up, we need to fix the tests.

I added entity operations links, so that on the list page you can jump right into a section.

Really looking forward to this change!
Let us know when you feel that the patch is ready to be tested. :)

Please test it out. I don't want to mark ready for review until the tests are fixed, though.

Fixing tests.

This patch additionally removes dependency on CTools in the entity_browser module.

It looks like I didn't fix all the functional tests, I was just looking at the /src/tests folder, not the /tests folder.

I think some of tests were failing because I failed to capitalize "Routing" in namespace. Updating patch to run tests.

Updating patch again, capitalizing "routing" > "Routing" in service declaration.

Fixing errors in tests.

Please test it out. I don't want to mark ready for review until the tests are fixed, though.

Alright I'll try it on a few sites this coming week.

Attempting to fix the last test error.

Fixing tests

OK, Tests are fixed. Marking ready for review.

Testing the patch against the 8.x-1.x branch.

Here's an updated patch.

The operations and tabs for the subforms now are hidden if the plugin lacks a configuration form. I'm rebuilding the access to these routes (and hence the local tasks) when the entity browser is saved. I think this slows the form submit though, which is annoying. Using local task alter doesn't help speed it up either, also, the caching permission really only works if you have an access callback.

The tabs for subforms that lack configuration form don't display:

The operations for subforms that lack configuration form don't display:

Rerolling for 8.x-2.x

Fixing a bug introduced in the last patch (typo on variable name in last builder).

Same bug fix as above, but for 8.x-1.x branch

Here's a new version of a reconfigured backend ui. This time, instead of four tabs, I combine the first three tabs into one form and load the subform in. When you update a plugin, the plugin settings form changes. This seems even more user friendly than four sets of tabs. I left the last tab where you configure widgets as a separate tab, as it is much larger than the other subforms.

Here's a screenshot of the new ui in patch #30.

And here's a screenshot with one of the subforms expanded.

Same as patch in #30 but against the 8.x-1.x branch.

This isn't really a meta issue is it?

It's still open to discussion. It'd be great if someone took a look at my work and gave some feedback.

I think you're on the right track, just replacing the wizard is a huge help. I like the wizard for creating new things, but they are a pain when it comes to editing existing things.

One change I would make is to the help text markup. Right now it is all just thrown into the fieldset. If we add some paragraphs and a definition list then the content would be more semantic and it would flow visually much better.

<p>
            Choose here how the browser(s) should be presented to the end user.</p><p> The available plugins are:</p>
    <dl><dt>iFrame:</dt> 
        <dd>Displays the entity browser in an iFrame container embedded into the main page.</dd>
        <dt>Modal:</dt>
        <dd> Displays the entity browser in a modal window.</dd>
        <dt>Standalone form:</dt>
        <dd> Displays the entity browser as a standalone form. Only intended for testing or very specific use cases.</dd></dl>

The screenshot is using the Material Admin Theme.

That was testing 1.4.0 + patch.

Here's a new version of the patch against 8.x-1.x branch, with new description format based on feedback from frob.

Same as patch 38 (including new description markup), but for the 8.x-2.x branch.

Here's an example of the add form.

Here are some screenshots of the entity browser add form (patch 38 and 40, vs 8.x-1.x and 8.x-2.x branches respectively). This is with Adminimal admin theme.

Here's the entity browser add form (also used for edit form):

With the display plugin settings collapsible fieldset open (using a "details" render element):

After switching the display plugin, the display plugin settings update via ajax:

And again after switching display plugin settings to the standalone display plugin.

The other plugins behave the same way, with collapsed "details" render elements that open when the plugin is switched (since that's when you'd presumable want to config it). This is also handy to get a sense of the options for each plugin.

Here's a summary of changes in the patch.

1) It removes the dependency on ctools, replace the multistep wizard with two forms, one for selecting and configuring plugins and a second for selecting and configuring widgets.

• /admin/config/content/entity_browser/{entity_browser}/edit
• /admin/config/content/entity_browser/{entity_browser}/widgets

2) It adds operations link for "Edit Widgets" to allow going straight from the entity list page to edit widgets.

3) It adds sub tabs to allow toggling between the main edit form and the widget edit form.

4) adds isConfigurable() to DisplayBase class so that plugins can indicate to the form if they are configurable without having to load the form and check if it's empty. It seemed checking a boolean is more efficient than having to load the form just to see if it's configurable.

5) remove PluginConfigFormBase and the various plugin config forms. These were basically an intermediary between the wizard and the plugin's configuration form hooks and aren't needed since you can validate and submit them using validateConfigurationForm and submitConfigurationForm on the plugin object. The tricky thing is pulling out a SubFormState, see buildEntity and validateForm in EntityBrowserEditForm.

6) Add new ajax functionality to main entity browser form to update the plugin configuration form for each of the three plugin types the entity browser supports. This makes separate tabs for configuring these plugins unnecessary and since these plugins often only have a few options and many have no options, it's more efficient and user-friendly to do it this way.

7) Revise the description text to be more semantic.

8) GeneralInfoConfig (the first part of the wizard) becomes the main entity form for entity browsers, EntityBrowserEditForm.

9) WidgetsConfig now extends EntityForm

10) in WidgetsConfig, moved the temp storage in ($form_state->getTemporaryValue('wizard')) to using \Drupal::service('tempstore.shared'), this is one area where we we might be able to improve the patch by using storage on the formState object. I originally wanted to keep the form as close to the original version to prevent regressions. But I think the tempstore service may be unnecessary and the changes could be store in the formstate object.

11) Set width and height parameter in IFrame display config to required

12) set the width and height text fields for the IFrame and Modal plugins to #size 10, since they were much bigger than anyone would ever need.

13) Updated tests to work with new UI.

Updating the test ConfingUITest based on https://www.drupal.org/project/entity_browser/issues/2966853

very slight change from 43, adding variable description

Reroll of patch 45 against 8.x-1.x branch

renaming a file to fix failure in test in Drupal 8.6 (see https://www.drupal.org/project/entity_browser/issues/2966853)

reroll for 8.x-1.x branch

reroll for 8.x-2.x branch

reroll for 8.x-2.x branch, fixing file name

reroll for 8.x-2.x branch, last one failed to apply

reroll for 8.x-1.x branch, last one failed to apply

I tested the patch in #54 and it makes the configuration (especially while creating a new entity browser) much faster.

Thanks for testing it. I've been using it for a couple of months. One thing I think we could add when you first create an entity browser and submit, it should automatically go to the widgets page. This would only be on the initial submit / creation of the entity browser.

@oknate +1 on #56

Here's an updated patch (for 8.x-2.x branch and 8.x-1.x branch) as well as an interdiff on the change. It now redirects to widget edit on entity browser initial creation.

A few tests need updating

still one fail, and it looks like a lot of drupal standards fixes need to be added.

added a fix for the one test that failed. Testing against Drupal 8.7.

Added some standards fixes and testing against 8.6

Adding some more standards fixes.

This patch gets the number of standards errors down from 79 down to 29 (in #61)

Adding some more standards fixes, down to 17 standards messages, many can't be changed because of backwards compatibility.

Rerolling against against latest 8.x-2.x branch.

Rerolling against current head.

Tried the UI and I think it looks really nice. The way the different plugin options are described is a bit uncommon but it looks good.

However, the patch is absolutely massive and based on a quick glance, a ton of things are not directly to this but are all kinds of cleanups.

It would help to split up everything that's clearly unrelated cleanup into a separate issue, then we can get that in first. I'd approach that by committing the patch to a branch and then do git checkout -p and pull in everything that is clearly standalone cleanup like the CSS stuff, documentation improvements and so on.

Thanks for the review. I created a separate issue for the coding standard changes #3031794: Coding Standard Fixes. I will remove them from the patch and post a new patch without them.

Here's an updated patch without the coding standard changes. I'm not going to test it right now, as all the tests are failing.

Well, testing actually makes sense, because this should fix most of the test fails by not relying on ctools anymore.

As expected, all tests are passing on 8.6 with the exception of the messed up update test, which is fixed in the other issue. Great.

Overall, I think this looks great, some feedback on ajax/tempstore stuff in the form.

1. +++ b/src/Form/EntityBrowserEditForm.php
   @@ -0,0 +1,457 @@
   +    if (empty($entity_browser->id())) {
   

   isNew() would be a bit cleaner here.

2. +++ b/src/Form/EntityBrowserEditForm.php
   @@ -0,0 +1,457 @@
   +      $help_text .= '<p>' . $this->t("Note: After you submit this form, you'll need to visit the other tabs. On this first form you need define the main characteristics of the Entity Browser (in other words, which plugins will be used for each functionality).  On the other tabs you will configure the plugins.") . '</p>';
   +      $help_text .= '<p>' . $this->t('You can find more detailed information about creating and configuring Entity Browsers at the <a href="@guide_href" target="_blank">official documentation</a>.', ['@guide_href' => 'https://drupal-media.gitbooks.io/drupal8-guide/content/modules/entity_browser/intro.html']) . '</p>';
   

   "the other tabs" is a bit strange considering there is now AFAIK only a single additional tab/local task?

   Can't we just redirect there by default when saving a new browser?

3. +++ b/src/Form/EntityBrowserEditForm.php
   @@ -0,0 +1,457 @@
   +      '#limit_validation_errors' => [],
   ...
   +  public static function submitUpdateDisplayPluginSettings($form, FormStateInterface $form_state) {
   +    $display = $form_state->getUserInput()['display'];
   +    $form_state->getFormObject()->getEntity()->setDisplay($display);
   +    $form_state->setRebuild();
   

   Using getUserInput() here is necessary because limit validation errors is an empty array. This is problematic as it is not safe, it could be any value.

   Instead, you should set limit_validation_errors so that only the select is validated and then you can access it with getValue:

   '#limit_validation_errors' => [['display']],
   

4. +++ b/src/Form/EntityBrowserEditForm.php
   @@ -0,0 +1,457 @@
   +    if ($widget_selector_plugin->isConfigurable()) {
   +      $widget_selector_config_form = $widget_selector_plugin->buildConfigurationForm([], $form_state);
   +      $form['widget_selector_wrapper']['widget_selector_configuration'] = array_merge($form['widget_selector_wrapper']['widget_selector_configuration'], $widget_selector_config_form);
   +    }
   +    else {
   +      $form['widget_selector_wrapper']['widget_selector_configuration']['no_options'] = [
   +        '#prefix' => '<p>',
   +        '#suffix' => '</p>',
   +        '#markup' => $this->t('This plugin has no configuration options.'),
   +      ];
   +    }
   

   isConfigurable() is a new method? Can't we just check whether $widget_selector_config_form is empty?

5. +++ b/src/Form/EntityBrowserEditForm.php
   @@ -0,0 +1,457 @@
   +      case 'display':
   +        $intro = $this->t('Choose here how the browser(s) should be presented to the end user.');
   

   we always configure one, so (s) is not needed? Also here it's just browser while below we use "entity browser", I think the second is better as it is more specific.

6. +++ b/src/Form/EntityBrowserEditForm.php
   @@ -0,0 +1,457 @@
   +   * {@inheritdoc}
   +   */
   +  public function buildEntity(array $form, FormStateInterface $form_state) {
   +
   

   In fact buildEntity() might just work then and you could possibly have a single submit that does nothing but a rebuild for all 3 cases.

7. +++ b/src/Form/EntityBrowserEditForm.php
   @@ -0,0 +1,457 @@
   +    // If this is a new entity, redirect to the widget edit form.
   +    if ($this->entity->isNew()) {
   +      $params = [
   +        'entity_browser' => $this->entity->id(),
   +      ];
   +      $form_state->setRedirect('entity.entity_browser.edit_widgets', $params);
   +    }
   

   Ah you even have that code here? So not sure why that description is necessary?

8. +++ b/src/Form/WidgetsConfig.php
   @@ -49,8 +49,30 @@ class WidgetsConfig extends FormBase {
   +
   +    $user_input = $form_state->getUserInput();
   +
        /** @var \Drupal\entity_browser\EntityBrowserInterface $entity_browser */
   -    $entity_browser = $form_state->getTemporaryValue('wizard')['entity_browser'];
   +    $entity_browser = \Drupal::routeMatch()
   +      ->getParameter('entity_browser');
   +
   +    if (empty($user_input)) {
   +      $tempstore->set($entity_browser->id(), $entity_browser);
   +    }
   +    else {
   +      $entity_browser_id = $user_input['entity_browser_id'];
   +
   +      /** @var \Drupal\entity_browser\EntityBrowserInterface $entity_browser */
   +      $entity_browser = $tempstore->get($entity_browser_id);
   +    }
   

   I never liked this pattern from ctools, because it means that simply by accessing that page, you create the tempstore data, afaik there is also no UI telling you about unsaved data?

   I prefer the approach in views, where the tempstore data is set on the first change, and then there's a warning message that points out that there is unsaved data, with an option to cancel.

9. +++ b/src/Form/WidgetsConfig.php
   @@ -173,13 +204,15 @@ class WidgetsConfig extends FormBase {
   -    $cached_values = $form_state->getTemporaryValue('wizard');
   +    $tempstore = \Drupal::service('tempstore.shared')
   +      ->get('entity_browser.config');
   +
   +    $entity_browser_id = $form_state->getUserInput()['entity_browser_id'];
   

   this part needs to be a bit more dynamic then, as it might not yet exist in tempstore but otherwise it shouldn't be much more complicated.

10. +++ b/src/Form/WidgetsConfig.php
    @@ -194,8 +227,13 @@ class WidgetsConfig extends FormBase {
    +
    +    $entity_browser_id = $form_state->getUserInput()['entity_browser_id'];
    +
    

    for the ID, I would use #type value and then $form_state->getValue(). Unlikely in an admin form but still, someone could mess things up real good by submitting a different value here.

Changing this to critical and giving a more specific, non-meta title.

Addressing some of the easier changes suggested in #76, (1, 2, 3, 4 and 5).

Still needs addressing: 6, 8, 9 and 10. Updated 8, 9 and 10 addressed in #81.

I'm not sure I understand what is being suggested in 6. I think to use rebuild entity in the ajax callbacks when changing plugins to reduce the amount of code.

Also
$tempstore = \Drupal::service('tempstore.shared')->get('entity_browser.config');

the tempstore shared should be injected in the constructor rather than called directly.

Fixing a typo in new verbiage.

The subform validation was running when switching plugins after adding the limit validation errors. Testing a fix.

- Injecting temp store for non-static methods in WidgetsConfig.
- removing automatic setting of tempstore on visiting widgets form, per #76 suggestion number 8
- updating the retrieval of tempstore data in WidgetsConfig, since this is an entity form now, we don't need to retrieve the entity browser id from user submitted data.

Oops, the logic was slightly off in #81, updating.

When tempstore is not cleared, (this would happen if you navigate away without saving), when you return, a warning is now displayed:

Fixing typos, it's weird this didn't break anything.

Adding an interdiff from #73 to #84 so one can see all the changes at once.

Tested manually a bit and I think it works well enough to commit it so we can unblock testbot and likely do a release then.

That said, I'm wondering if we need the tempstore at all for this. We no longer have complex multistep processes, there's no preview functionality that would need to access the altered config on a separate request like views. At this point, it's pretty close to the core entity form/view display configuration and that works just fine with repeated, regular ajax updates/form rebuilds until we eventually save it.

Another possible improvement would be to make sure that the "there are unsaved changes" already shows up after making any change, so that its clear that it is not saved yet.

Thoughts? Happy to try to look into that in a follow-up and getting it in like this now.

I think the tempstore is left over from the ctools wizard. So I think it would be great to remove it. But I think getting stable builds should come first. We can create a separate issue to remove it.

I also agree about the unsaved changes. But if we can remove the tempstore, we won't even need that message.

It is a ctools thing, but it is also basically just custom code now, so as long as we keep $this->entity up to date, it should just work without it.

I think we won't commit this tonight anymore anyway (pretty late already for me and Primoz) so if you want to give removing that a try, go for it :)

OK, If I have time tonight, I'll see if I can get it working without it.

Here's an update that removes the tempstore factory from the WidgetsConfig form. That was a good suggestion, it's totally unnecessary.

The only surprising thing was that deleting a widget was taking affect immediately, but that was easy to track down, the EntityBrowser::deleteWidget method was saving the EntityBrowser. I checked where the method is used, and it's only used in WidgetsConfig, so it's safe to update it. I suppose it's an API change, so we should document it, but I doubt it would break any 3rd party integrations.

+++ b/src/Form/WidgetsConfig.php
@@ -65,20 +54,6 @@ class WidgetsConfig extends EntityForm {
-      $form['changed'] = [
-        '#type' => 'container',
-        '#attributes' => ['class' => ['view-changed', 'messages', 'messages--warning']],
-        '#children' => $this->t('You have unsaved changes.'),
-        '#weight' => -10,
-      ];
-    }

Something like this would still be useful IMHO, but we can look into that in a follow-up. Might be a simple as adding that as a real message, the ajax system should take care of displaying that above the replaced part automatically.

Looks great to me and great work!

I just have a few nitpicks.

1. +++ b/src/Form/EntityBrowserEditForm.php
   @@ -0,0 +1,453 @@
   +  protected function getPluginDescription($plugin_type = 'string') {
   

   Couldn't this be just '' instead of 'string'.

2. +++ b/src/Form/EntityBrowserEditForm.php
   @@ -0,0 +1,453 @@
   +    $output = "<p>$intro</p>";
   +    $output .= '<p>' . $this->t('The available plugins are:') . '</p>';
   

   This part and below could be a render array, but not super important.

3. +++ b/src/Form/EntityBrowserEditForm.php
   @@ -0,0 +1,453 @@
   +    $subFormState = SubformState::createForSubform($form['display_wrapper']['display_configuration'], $form, $form_state);
   

   No camel casing.

4. +++ b/src/Form/EntityBrowserEditForm.php
   @@ -0,0 +1,453 @@
   +    $subFormState = SubformState::createForSubform($form['display_wrapper']['display_configuration'], $form, $form_state);
   

   Same.

Maybe we could also remove the ctools downloading step from travis-before.sh.

Updated patch based on feedback in #95.

Great work! Thank you.

Oh,.. for 8.x-1.x needs a re-roll.

Reroll against 8.x-1.x branch.

Really exciting, thanks for the great work.

Nice, committed.