Problem/Motivation

In #1966424: Change notice: Allow Views handlers to be optional we allowed handlers to be denoted as optional, so that if their providing module was broken, the Views UI would still function.

When a missing handler is optional, we should provide better UI text.

Proposed resolution

Now people see "broken handler" but have no context at all, so let's show them the information we know

  • The missing plugin
  • The missing module

One important bit is that we have to show the information as fast as possible, because a broken handler is a clear sign that something is seriously broken (as indicated). Hiding information causes problems on the longrun.

This is blocked on #1825896: Add module owner to plugin data on handlers and is part of #1822048: Introduce a generic fallback plugin mechanism

Original report by [username]

(Text of the original report, for legacy issues whose initial post was not the issue summary. Use rarely.)

Files: 
CommentFileSizeAuthor
#51 2016953-51.patch29.9 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 58,755 pass(es).
[ View ]
#51 interdiff-2016953-51.txt21.32 KBdamiankloip
#51 2016953-51.png40.92 KBdamiankloip
#48 2016953-48.patch28.23 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 58,469 pass(es).
[ View ]
#48 interdiff-2016953-48.txt8.03 KBdamiankloip
#44 vdc-2016953-44.patch27.59 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch vdc-2016953-44.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#34 edit.png7.62 KBdawehner
#34 dialog.png15 KBdawehner
#34 vdc-2016953-34.patch23.57 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 57,699 pass(es), 39 fail(s), and 28 exception(s).
[ View ]
#34 interdiff.txt17.49 KBdawehner
#32 broken_vs_optional.png47.54 KBxjm
#24 before_patch.png56.78 KBxjm
#24 after_patch.png66.48 KBxjm
#24 optional_handler_modal.png39.22 KBxjm
#22 2016953-22.patch16.39 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 57,739 pass(es).
[ View ]
#22 interdiff-2016953-22.txt874 bytesdamiankloip
#20 2016953-20.patch16.39 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] 57,947 pass(es), 8 fail(s), and 0 exception(s).
[ View ]
#20 interdiff-2016953-20.txt8.2 KBdamiankloip
#15 vdc-2016953-15.patch15.17 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 57,280 pass(es).
[ View ]
#15 interdiff.txt13.02 KBdawehner
#13 vdc-2016953-13.patch12.29 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 57,551 pass(es).
[ View ]
#9 interdiff.txt1.89 KBdawehner
#9 vdc-2016953-9.patch12.11 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 56,374 pass(es).
[ View ]
#7 drupal-2016953-7.patch7.43 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 57,906 pass(es), 32 fail(s), and 32 exception(s).
[ View ]
#7 interdiff.txt2.41 KBdawehner
#4 broken-optional-2016953-3.patch5.02 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 56,152 pass(es).
[ View ]
#2 Screen Shot 2013-06-12 at 1.00.32 PM.png39.3 KBtim.plunkett
#1 broken-optional-2016953-1.patch6.53 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 57,293 pass(es), 0 fail(s), and 6 exception(s).
[ View ]

Comments

tim.plunkett’s picture

Status:Postponed» Needs review
StatusFileSize
new6.53 KB
FAILED: [[SimpleTest]]: [MySQL] 57,293 pass(es), 0 fail(s), and 6 exception(s).
[ View ]

Well #1825896: Add module owner to plugin data on handlers will allow us to make this more helpful, but we can start making progress first.

tim.plunkett’s picture

StatusFileSize
new39.3 KB

Screen Shot 2013-06-12 at 1.00.32 PM.png

Status:Needs review» Needs work

The last submitted patch, broken-optional-2016953-1.patch, failed testing.

tim.plunkett’s picture

Status:Needs work» Needs review
StatusFileSize
new5.02 KB
PASSED: [[SimpleTest]]: [MySQL] 56,152 pass(es).
[ View ]

Different approach. Interdiff was useless.

dawehner’s picture

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/area/Broken.phpundefined
@@ -19,7 +19,7 @@
-    return t('Broken/missing handler');
+    return $this->optional ? t('Optional handler is missing') : t('Broken/missing handler');

Can't we pull in the plugin ID here?

tim.plunkett’s picture

At this point the plugin ID is 'broken'. We could pass more information in the $manager->createInstance('broken') call, but I that's not specific to optional ones, and we should fix that in the bigger broken issue.

dawehner’s picture

StatusFileSize
new2.41 KB
new7.43 KB
FAILED: [[SimpleTest]]: [MySQL] 57,906 pass(es), 32 fail(s), and 32 exception(s).
[ View ]

Let's write a test for it.

Status:Needs review» Needs work

The last submitted patch, drupal-2016953-7.patch, failed testing.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new12.11 KB
PASSED: [[SimpleTest]]: [MySQL] 56,374 pass(es).
[ View ]
new1.89 KB

This could fix it (together with the missing yml files).

damiankloip’s picture

I think we should still try to get #1825896: Add module owner to plugin data on handlers in first, so we can add the module dependency with this too. This would be a bit more useful I think, at the moment we are just displaying a different generic message.

dawehner’s picture

Status:Needs review» Postponed

So

damiankloip’s picture

I resumed progress on linked issue in #10

tim.plunkett’s picture

Assigned:tim.plunkett» Unassigned
StatusFileSize
new12.29 KB
PASSED: [[SimpleTest]]: [MySQL] 57,551 pass(es).
[ View ]

Rerolled, but it needs an update now that #1825896: Add module owner to plugin data on handlers is in

jibran’s picture

Status:Postponed» Needs review
dawehner’s picture

StatusFileSize
new13.02 KB
new15.17 KB
PASSED: [[SimpleTest]]: [MySQL] 57,280 pass(es).
[ View ]

Let's try to leverage as many information as possible and present them to the user.

xjm’s picture

Issue tags:+Usability

Agreed @dawehner, that will make it easier for people to debug.

This has a usability impact, because telling people Drupal is broken right after they install is a WTF.

xjm’s picture

Issue tags:+Needs manual testing

'Cause it's Views.

dawehner’s picture

Thank you for treating people different, every patch should have a manual testing flag tbh.

tim.plunkett’s picture

Status:Needs review» Needs work

Undefined index: provider in Drupal\views\Plugin\views\field\Broken->adminLabel() (line 23 of core/modules/views/lib/Drupal/views/Plugin/views/field/Broken.php).

ViewsHandlerManager::getHandler() passes through $item, but it doesn't seem to always have the provider key set.

damiankloip’s picture

Status:Needs work» Needs review
StatusFileSize
new8.2 KB
new16.39 KB
FAILED: [[SimpleTest]]: [MySQL] 57,947 pass(es), 8 fail(s), and 0 exception(s).
[ View ]

Gave this a test run, works as expected. Just a couple of things:

- The replacement for @field was wrong though ('@Field')
- The content translation link just didn't have a provider key in its default config
- Also, I think we should add an isOptional() method; then people can determine this with other logic. Rather than having to add this to their configuration (we could even look at that, although might involve checking is a module exists :/)

Status:Needs review» Needs work

The last submitted patch, 2016953-20.patch, failed testing.

damiankloip’s picture

Status:Needs work» Needs review
StatusFileSize
new874 bytes
new16.39 KB
PASSED: [[SimpleTest]]: [MySQL] 57,739 pass(es).
[ View ]

Forgot to fix the test string too.

dawehner’s picture

Status:Needs review» Reviewed & tested by the community

Ha. I think this is ready to fly now.

xjm’s picture

Issue tags:-Needs manual testing+Needs usability review
StatusFileSize
new39.22 KB
new66.48 KB
new56.78 KB

Manually tested and it works fine. Screenshots attached.

I think we might want a usability review here, though. It's a lot of text. Not bumping back to NR; I'll let a maintainer make that call if the UX team doesn't get to it first.

Before patch

before_patch.png

After patch

after_patch.png

Modal you get when clicking the link (unchanged)

optional_handler_modal.png

xjm’s picture

Seriously, d.o.

webchick’s picture

Status:Reviewed & tested by the community» Needs review

Hm. This makes the message a lot longer and more technical than it was before. I'd expect it to just say "Optional handler missing" and not expose the technical details unless I clicked on it to reveal more.

Tossing back to needs review for a moment to see what the stance is from the UX team on that.

xjm’s picture

The specifics of which module etc. provides the hander are very valuable; I wonder if it would make sense to put them on the modal you get when you click the link, or reveal them in another way?

dawehner’s picture

Added an issue summary adressing some points ... Note the part about hiding. If you are scared about the message then it is the best thing we could have achieved ... a broken handler is a sign of a problem, so people will deal with it fast.

webchick’s picture

Right, but normally we indicate problems with yellow/red backgrounds, warning icons, and the like. Not with one table row mysteriously 3 lines high vs. the one row high that everything else surrounding it is.

I agree the extra debugging info is valuable, I would just put it behind a click. Else my eye's going to have to hit that extraneous info every single time I tweak the content view throughout the life of my site, even if I never, ever intend to make my site multilingual.

tim.plunkett’s picture

Can we compromise and do "Overridden @module handler is missing" and have the field/table in the modal?

webchick’s picture

I would be fine with that, yep. Might want to get the opinion of the UX team.

xjm’s picture

StatusFileSize
new47.54 KB

I agree that the module name is the most important information. Most likely scenario is "Oh, this is because I uninstalled Foo module."

I realize I forgot to note that the screenshots in #24 are editing admin/content, in HEAD, on a fresh installation. We don't want the user to be scared about that; that's half the point of this issue.

I deliberately "broke" the field handler for the content type (by deleting it) to see what it would look like:

broken_vs_optional.png

damiankloip’s picture

Is the screenshot from an older patch? As the field token is not correctly replaced. This should be fixed from #21/#22.

dawehner’s picture

StatusFileSize
new17.49 KB
new23.57 KB
FAILED: [[SimpleTest]]: [MySQL] 57,699 pass(es), 39 fail(s), and 28 exception(s).
[ View ]
new15 KB
new7.62 KB

Is there any way how we could indicate the user to click on the link, because otherwise the issue is kind of pointless, as said often.

edit.pngdialog.png

... Hopefully this works.

xjm’s picture

Let's get some help from the UX team. The issue is tagged so it's just a matter of waiting for their input. :)

xjm’s picture

@damiankloip, it was the patch from #22.

Status:Needs review» Needs work

The last submitted patch, vdc-2016953-34.patch, failed testing.

tim.plunkett’s picture

damiankloip’s picture

But the image in #32 has no it had the field token replaced, and I'm pretty sure the patch in #22 fixed that. Its not a bid deal though :-) easily fixed.

dawehner’s picture

Let's not fix the test failures until someone decides how it should look like.

Bojhan’s picture

Sorry for getting to this issue so late, sadly few people watch that tag. I agree with @webchick here that we should omit the message from initial view. Since its likely this will wrap several lines, it will negatively impact scanability.

I understand the concern, that people might not click this - adding ellipses as suggested will solve this. I also suggest moving the error message from the title to the actual body (the modal redesign, should solve some of the visibility issues there).

The error message should read something like :

The handler for this item is broken or missing. You are missing handlers for the following parts:

- Module: language
- Table: node
- Field: langcode

Enabling the appropriate modules may solve this issue, otherwise removing could help.

Ideally the suggestions are a little less silly.

Bojhan’s picture

Removing tag

dawehner’s picture

Great feedback!

dawehner’s picture

StatusFileSize
new27.59 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch vdc-2016953-44.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Testing strings is sooooo annoying.
The tests aren't adapted yet

damiankloip’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, vdc-2016953-44.patch, failed testing.

damiankloip’s picture

Assigned:Unassigned» damiankloip

Rerolling and working on this

damiankloip’s picture

StatusFileSize
new8.03 KB
new28.23 KB
PASSED: [[SimpleTest]]: [MySQL] 58,469 pass(es).
[ View ]

Changed a few things, ViewEditFormController was not using the conditional code it had for broken handlers properly. changed the test_view_optional yaml file, and a few other things.

Oh, and the first drupalGet() in each test method needed to move inside the foreach() otherwise we only get the first link.

Didn't add any assertions for the body text. I think asserting the whole text is totally nuts, we could just check the tokens have been replaced correctly maybe, what do you think?

dawehner’s picture

Assigned:damiankloip» Unassigned
Status:Needs work» Needs review

damiankloip++

  1. +++ b/core/modules/views_ui/lib/Drupal/views_ui/Tests/HandlerTest.php
    @@ -74,7 +74,7 @@ protected function viewsData() {
    -  public function ptestUICRUD() {
    +  public function _testUICRUD() {

    HAHA

  2. +++ b/core/modules/views_ui/lib/Drupal/views_ui/Tests/HandlerTest.php
    @@ -148,25 +148,27 @@ public function ptestUICRUD() {
    +      $this->drupalGet('admin/structure/views/view/test_view_broken/edit');

    @@ -174,24 +176,27 @@ public function testBrokenHandlers() {
    -    $this->drupalGet('admin/structure/views/view/test_view_optional/edit');
    ...
    +      $this->drupalGet('admin/structure/views/view/test_view_optional/edit');

    Why do we need one drupalGet foreach handler type?

damiankloip’s picture

Why do we need one drupalGet foreach handler type?

Because atm there is another drupalGet for the actual config item form within the loop.

damiankloip’s picture

StatusFileSize
new40.92 KB
new21.32 KB
new29.9 KB
PASSED: [[SimpleTest]]: [MySQL] 58,755 pass(es).
[ View ]

Ok, I've made a few more changes to this...

Let's please get this in sooner rather than later, I (we) don't want endless iteration and bikeshedding on the exact text here, there is other useful stuff contained in this patch that would be good to get in. If some one wants to open an issue to talk about the exact text in detail, that could be an easy follow up issue?

I have changed the form description markup to use a render array, as well as adding an item list for the info too.

I've also attach a screenshot of a current example when this patch is used.

Bojhan’s picture

The text looks fine to me now, I dont think its bikeshedding at all. These are very actual improvements to the usability of that text.

damiankloip’s picture

Great, glad it seems ok. That came across wrong maybe; I just meant that if we wanted alot more time to discuss the text, it should move to it's own issue (potential bikeshedding, not previously in the issue). It seems you're happy with this too though :)

tim.plunkett’s picture

Status:Needs review» Reviewed & tested by the community

I think the text is great, I'm glad @Bojhan agrees!
From a technical standpoint this patch is solid.

webchick’s picture

Status:Reviewed & tested by the community» Fixed

Ok great. My earlier feedback is now resolved by making the overview table say:

Optional handler is missing (Module: foo) …

or else:

Broken/missing handler (Module: foo) …

Clicking on those gives you all the juicy details you could want. And since no other table rows use the ellipsis, hopefully that will entice people to click them and see what's what.

Committed and pushed to 8.x. Thanks!

damiankloip’s picture

Nice, thank you!

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

Anonymous’s picture

Issue summary:View changes

added issue summary