Adding a field with relationship results in confusing error message when relationship is not present

Reproduced in Drupal core 8.4.x-dev.

I will try it.

I think a message like 'The {filter} uses a relationship that is not present in any display' which is more explicative.

Fixed

+++ b/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
@@ -2458,7 +2458,7 @@ public function validate() {
+          $errors[] = $this->t('The %handler_type %handler uses a relationship that is not present in any display.',['%handler_type' => $handler_type_info['lstitle'],'%handler' => $handler->adminLabel()]);

Reading the steps to reproduce it appears that the relationship is present in at least one display so I'm not sure the "in any display" is correct either.

@andriy_novak thank you for reviewing this issue! A short RTBC message liked "Fixed" from a first time commenter on the issue leaves a lot on the committer to check.

What we do need people to review is whether the issue has a correct scope, whether it passes the core gates, whether the solution completely fixes the problem without introducing other problems, and whether it's the best solution we can come up with. See the patch review guide for more information.

When you do post a review, be sure to describe what you reviewed and how. This helps other reviewers understand why you considered the issue RTBC (and is considered for issue credit).

Hi @alexpott thanks for review message. I agree with you and I think a message like '... uses a relationship which is not present in all displays' would be better.

+++ b/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
@@ -2458,7 +2458,7 @@ public function validate() {
-          $errors[] = $this->t('The %handler_type %handler uses a relationship that has been removed.', ['%handler_type' => $handler_type_info['lstitle'], '%handler' => $handler->adminLabel()]);
+          $errors[] = $this->t('The %handler_type %handler uses a relationship which is not present in all displays.',['%handler_type' => $handler_type_info['lstitle'],'%handler' => $handler->adminLabel()]);

Whilst we're improving the message - do we think it is worth telling the user which relationship is missing?

I think it would be a good idea to help user to find which realtionship cause that error.

The error still happens in 8.5.6 (I mean, this f*** error in which I lost 2 hours, still happens in Drupal 8.5.6).

The patch seems correct, thanks @megan_m for the report and all collaborators for the fix. I think that we can add the relationship name and the display name that cause the error.

Something like:

    $display = $this->view->getDisplay();
    $display_name = $display->display['display_title'];

$errors[] = $this->t('The %relationship_name relationship used in %handler_type %handler is not present in %display_name', ['%relationship_name' => $handler->options['relationship'], '%handler_type' => $handler_type_info['lstitle'], '%handler' => $handler->adminLabel(), '%display_name' => display_name]); 

+++ b/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
@@ -2458,7 +2458,7 @@ public function validate() {
+          $errors[] = $this->t('The %handler_type %handler uses a relationship which is not present in all displays.',['%handler_type' => $handler_type_info['lstitle'],'%handler' => $handler->adminLabel()]);

I don't think this message will always be correct either.

What about the case the original message covers, where a relationship is deleted?

+1 to the suggestion in #17.

(Also, missing space after the comma.)

Yeah #17 sounds pretty good, updated the patch and the test to use that. Also updated the test not to use t().

The end result looks something like this:

Added a period to the end of the error.

And now with the patch attached.....

LGTM.

Though, just a nitpick -- I would split the array in the t() over multiple lines for readability.

+++ b/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
@@ -2522,7 +2522,7 @@ public function validate() {
+          $errors[] = $this->t('The %relationship_name relationship used in %handler_type %handler is not present in %display_name.', ['%relationship_name' => $handler->options['relationship'], '%handler_type' => $handler_type_info['lstitle'], '%handler' => $handler->adminLabel(), '%display_name' => $this->display['display_title']]);

I think this should say '%display_name display', as the display name isn't always clear to understand in a sentence.

And a nitpick: the t() array would be more readable split over multiple lines now that it's so much longer.

Thanks for the review! Changes sound good, implemented here.

Edit: Oh didn't do the multiline thing for the test, but it's a test so ¯\_(ツ)_/¯

LGTM!

The code looks good, just wondering however if we can auto-detect the user is about to do this and prevent them from adding the field/filter/sort to 'all displays' when the relationship doesn't exist on the given display? i.e preventing the foot gun rather than warning about it? If that sounds like a follow-up task/feature, please put back to RTBC @Lendude

Honestly, I think that the current solution fixed the original bug "a confusing message" and it's quickly to be applied.

Maybe is better to close the issue right now with a valid solution and open a new one to discuss if we can manage the situation of another way.

I agree with #34.
I think we should close this to fix the situation and discuss it in a new issue.

@larowlan I get what you are saying, but I think I remember this was discussed when we added this message too. Views had always allowed you to shoot yourself in the foot (and any other body part you can think of...). People have all sorts of different workflows when working on Views and not allowing them to do something that can be fixed later will mess with peoples workflow and lead to annoyance when we stop allowing people to work the way they are used to.

So I think we should hold to the pattern that we allow site builders to do what they want.

Yeah I agree with the replies here, this normally happens when you have a mismatch between 'default' and 'overridden' for different parts of the view, and we can't know whether it's the fields or the relationships where the mistake is, so just informing people seems better than validating - knowing you need to click out, go and change something elsewhere, then click back into make the change isn't necessarily straightforward for people.

Committed/pushed to 10.1.x, cherry-picked to 9.5.x and 9.4.x. Since it's a string change, leaving the 9.4.x backport. Thanks all!