Problem/Motivation

At the moment the Configuration Selector works in the background picking the best configuration for you based on priority. The user can go to each specific admin interface (for example views UI) and disable and enable views to change which configuration is selected from a feature but this:

  • clunky
  • hard to discover

Proposed resolution

Add a UI for selecting different configuration form a feature. To do this we'll add a new configuration entity to describe a feature. This is so that

  • Initially we have a description. Features with no corresponding feature entities are excluded from the UI. This also helps with no reading all the configuration.
  • Later we can use this plumbing to allow developers to built out Configuration Selector features in UI.

Remaining tasks

Write tests.

To test manually:

User interface changes

There is a UI. There was not before.

API changes

New API for Configuration Selector feature entity.

Data model changes

None

Comments

alexpott created an issue. See original summary.

alexpott’s picture

Status: Active » Needs review
Issue tags: +Needs tests
StatusFileSize
new19.64 KB

Here's a start. Needs tests though.

alexpott’s picture

daniel.bosen’s picture

Do we need a new third party setting for a description? Right now, when configuring the a specific feature, I can not distinguish the given alternatives. The feature name is shown, but this is the same for all configs.

alexpott’s picture

@daniel.bosen good point - I think we do need something. So people know what they are choosing between. Maybe we could try an use the view description and block admin description for example.

daniel.bosen’s picture

@alexpott thats even better then adding a new description option, it would also be beneficial outside the selector UI to have those descriptions in good shape.

alexpott’s picture

Issue tags: -Needs tests
StatusFileSize
new17.9 KB
new30.76 KB

Here's #5 implemented. Unfortunately I still think we need a fallback because not all config ie blocks supports descriptions. But views do and it works great.

Also included are tests for the UI.

Status: Needs review » Needs work

The last submitted patch, 7: 2948788-7.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

alexpott’s picture

Status: Needs work » Needs review
StatusFileSize
new1.42 KB
new30.74 KB

Hmmm... let's use a bigger hammer.

mtodor’s picture

I have tested UI a bit. It looks nice and it makes a general idea more useful and transparent to the user - I like it.

There are few things that I have encountered. Maybe it's not directly related to UI, but it's easier to notice it now.

  1. When we have blocks and views in one feature. There are two tables and columns are not nicely aligned, that looks a bit ugly. Is it possible to join results in one table? with headers for every of config type (if I can call it like that)
  2. Some kind of sorting in the table of available configurations for the feature would be nice - I would suggest priority order
  3. One use case: I have two blocks and they are both activated in blocks UI. When I go to the configuration selector feature management page, they are both enabled and I don't have the option to disable any of them, just to edit them. I'm not sure, should we provide disable option or promote option (that's our select option) since configuration state (according to configuration selector behavior) is not in the correct state. I'm not sure if we can or should address this case?
  4. This error occurred for me. I have tested it on Search API PR from Thunder: https://github.com/BurdaMagazinOrg/thunder-distribution/pull/434 - I enabled Search API and Media View for it was enabled. So there are 2 views for one feature. Then I deleted active view and my site crashed because a view is not available. Should config selector jump in and set an available view for that feature as active or we are not planning to handle explicit deletion of config?
  5. When I delete all configurations related to one feature, feature still exists in list, but manage page is empty. Question is, should we hide features without any available configuration or not? Or maybe just disable manage configuration button for that feature, what would be also nice solution?

I have found just a few code nitpicks:

  1. +++ b/src/Controller/Switcher.php
    @@ -0,0 +1,80 @@
    +        $entity->setStatus(FALSE);
    

    It would be nice to add logging for all configs where status is changed.

  2. +++ b/src/Entity/Feature.php
    @@ -0,0 +1,110 @@
    +    foreach (\Drupal::entityTypeManager()->getDefinitions() as $entity_type) {
    ...
    +    $entity_storage = \Drupal::entityTypeManager()->getStorage($entity_type_id);
    

    Can we use $this->entityTypeManager() here?

  3. +++ b/tests/modules/config_selector_ui_test/config_selector_ui_test.install
    @@ -0,0 +1,57 @@
    +  $node_optional_dir = drupal_get_path('module', 'node') . '/config/optional';
    +  $node_config_storage = new FileStorage($node_optional_dir);
    +  $view_data = $node_config_storage->read('views.view.content');
    

    Nice one!

  4. +++ b/tests/src/Functional/ConfigSelectorUiTest.php
    @@ -0,0 +1,105 @@
    +    $view_storage = \Drupal::entityTypeManager()->getStorage('view');
    

    It would be nice to move \Drupal::entityTypeManager() into variable.

  5. +++ b/tests/src/Functional/ConfigSelectorUiTest.php
    @@ -0,0 +1,105 @@
    +    $this->assertEquals('Test configuration', $this->xpath('//table[1]/thead/tr[1]/th[1]')[0]->getText());
    +    $this->assertEquals('View', $this->xpath('//table[2]/thead/tr[1]/th[1]')[0]->getText());
    

    Since we are using [@id="edit-table"] everywhere it would be better to use same selector here, just to reduce confusion. I think it's: "edit-table" and "edit-table--2".

alexpott’s picture

StatusFileSize
new4.88 KB
new31.47 KB

Thanks for the review @mtodor!

Re nitpicks:

  1. Good idea - implemented
  2. +1 fixed
  3. Thanks!
  4. I did this because of the container rebuilds. I'd prefer to use \Drupal here to be safe.
  5. Good idea.

Going to address the other more important parts of #10 next.

alexpott’s picture

@mtodor great questions in #10. Here's some thoughts. I'll start work on 1,2 and 5.

  1. I guess we can do that. It'll just mean a bit more custom styling for the module. I don't think we have anything like that to leverage for core's UI.
  2. I agree sorting by descending priority makes sense.
  3. Hmmm... both being enabled. That's kinda out-of-scope for the config selector. I think in that instance we should display an error to the user and allow the users to select one - in order to "fix" the situation. I think we should handle this in a follow-up so we can concentrate on getting the UI right for this edge case. Opened #2968912: Handle edge case where two configurations from the same feature are enabled in the UI
  4. I don't think the config selector should jump in and enable on delete but I do think we should set a message for a user and tell them about the possibility of enabling a replacement - let's discuss this in a follow-up too. Opened #2968915: Users can delete selected configuration - what should the config selector do?
  5. I think we should display an empty text on the manage configuration form - no point in hiding it.
alexpott’s picture

StatusFileSize
new94.07 KB
new14.29 KB
new36.1 KB

Here's points 1,2 and 5 addressed from #10.

For point 1 - I took a leaf out of the permissions UI (I know) and used colspan and indentation to group the entity types. I think it looks okay and only involves very very light CSS.

Status: Needs review » Needs work

The last submitted patch, 13: 2948788-13.patch, failed testing. View results

alexpott’s picture

Status: Needs work » Needs review
StatusFileSize
new2.1 KB
new36.36 KB

Let's make this pass regardless of the underlying Drupal 8.x version.

mtodor’s picture

Status: Needs review » Reviewed & tested by the community

I have tested changes and checked also code. It looks good to me and from my point of view, we can commit it.

P.S. There are few drupal coder styles that could be adjusted, but that can be done before commit.

chr.fritsch’s picture

Crediting @mtodor for reviewing

  • chr.fritsch committed 28a588d on 8.x-1.x authored by alexpott
    Issue #2948788 by alexpott, mtodor: Add a user interface so that users...
chr.fritsch’s picture

Status: Reviewed & tested by the community » Fixed

Fixed

Status: Fixed » Closed (fixed)

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