Indicate when an optional handler is missing, that it is not "broken"

+++ 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?

Let's write a test for it.

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

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.

So

I resumed progress on linked issue in #10

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

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.

'Cause it's Views.

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

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 :/)

Forgot to fix the test string too.

Ha. I think this is ready to fly now.

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

After patch

Modal you get when clicking the link (unchanged)

Seriously, d.o.

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.

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?

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.

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.

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

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:

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

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.

... Hopefully this works.

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

@damiankloip, it was the patch from #22.

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.

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

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.

Removing tag

Great feedback!

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

Rerolling and working on this

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?

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?

Why do we need one drupalGet foreach handler type?

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

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.

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.

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 :)

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!

Nice, thank you!