Views allows removal of required relationships and gives a fatal error on save

Unable to reproduce this issue with drupal 8.0-alpha10 release.

I just tried to reproduce this as well and couldn't. the steps i attempted were:

1) fresh 8.x-dev install
2) create content view
3) add the user-created relationship
4) add the fields user->created and user->last login
5) delete the relatinship
6) preview

I'm not sure that deleting the view is what you actually meant for step 4 since you can't delete it, then click save (its already gone). If there's a step missing please let us know.

My steps to reproduce weren't without error though. What does happen however is a SQL error is thrown because the table the fields need doesn't exist anymore. It might make sense to have the relationship not be removable until fields that depend on it (and only it) are removed.

Thoughts?

We should do two things here:

• Fix Field::access() to not fail completly when the exception is thrown but rather return FALSE. This is access related code, so we have to be safe
• Add a validation to DisplayPluginBase::validate() to check that there is no field with an invalid relationship. This will block the user from saving the view

here's what I get when following #2:

Drupal\Core\Database\DatabaseExceptionWrapper:
Exception in issue2204037[issue2204037]: SQLSTATE[42S22]: Column not found: 1054 Unknown column 'login' in 'field list': SELECT node_field_data.title AS node_field_data_title, node.nid AS nid, node_field_data.created AS node_field_data_created, created AS created, login AS login FROM {node} node INNER JOIN {node_field_data} node_field_data ON node.nid = node_field_data.nid WHERE (( (node_field_data.status = :db_condition_placeholder_0) AND (node.type IN (:db_condition_placeholder_1)) )) ORDER BY node_field_data_created DESC LIMIT 10 OFFSET 0; Array ( [:db_condition_placeholder_0] => 1 [:db_condition_placeholder_1] => page ) in Drupal\views\Plugin\views\query\Sql->execute() (line 1446 of core/modules/views/src/Plugin/views/query/Sql.php).

I wonder if this is ok:

- '#limit_validation_errors' => array(array('override')),
+ '#limit_validation_errors' => ($type == 'relationship') ? array(array('override'), array(NULL)) : array(array('override')),

Also noticed a strange behaviour:
Set views as described in #2 and then remove the relationship - "last login" will be broken but "created" will show content's created date. Obviously it happens because content and user entities both have "created" property.

Regarding the previous test fail and the fix:
Is it a fair assumption that if: (!empty($this->options['relationship'])) {
then: $relationships = array_keys($this->view->display_handler->getHandlers('relationship')); won't be undefined, i.e. view and display_handler will be set for the current instance?

fixed views test

Interesting work, will have a deep look in it tomorrow! Thank you for your great work!

Also have a look at notice at #5. Maybe it's a subject for a separate issue. I mean, it could point at some architectural drawbacks.

1. +++ b/core/modules/views/src/Plugin/views/HandlerBase.php
   @@ -473,6 +473,15 @@ public function showExposeForm(&$form, FormStateInterface $form_state) {
   +
   +    // Check for missing relationships.
   +    if (!empty($this->options['relationship'])) {
   +      $relationships = array_keys($this->view->display_handler->getHandlers('relationship'));
   +      if ($this->options['relationship'] != 'none' && !in_array($this->options['relationship'], $relationships)) {
   +        return FALSE;
   +      }
   +    }
   

   So in wonder whether we really have to do that on runtime or can't just always do that on save time.

2. +++ b/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
   @@ -2423,6 +2423,17 @@ public function validate() {
   +          $errors[] = $this->t('You can not remove this relationship because it is used by other handlers.');
   

   Can we list which handlers are using it?

One of the tests in #21 (views/src/Tests/Plugin/DisplayTest::testDisplayPlugin()) failed because ViewExecutable::getHandlers($type, $display_id) sets current display to $display_id (if given), and doesn't restore it to the previous value before return. Currently this was fixed by changing the test. If it is a bug, I suppose, it should be fixed in a separate issue.

upd: Added a follow-up issue #2408613: ViewExecutable::getHandlers() should restore display_id before return.

added a minor comment

Removed test fix (since #2408613: ViewExecutable::getHandlers() should restore display_id before return was fixed).
Imo, this one is ready for final review.

Its great to have some kind of test coverage here.

+++ b/core/modules/views/src/Entity/View.php
@@ -298,6 +298,32 @@ public function calculateDependencies() {
 

Honestly: I would rather move the logic into the display than into the view storage object, which once had the idea to just store stuff, and be kinda stupid otherwise. At least it would be nice to extract all the logic below into a method.

Hi,

I have reviewed the patch and it works when I remove the relationship from the "Configure Relationship" Form.

Unfortunately In "Rearrange relationships" form it allows to remove, unless it has dependencies, so the user is forced to cancel the submit form to revert the relationship removal.

@ivan.chavarro
Thanks, added "rearrange" form validation.

The point in #29 though is still true :(

I applied the patch and it works for me in both the Rearrange Relationship form and the Configure Relationship form. Thanks!

@dawehner
What kind of test coverage did you mean in #29 ?

Reviewing this in the mentored core sprints. Contributor tasks:
https://www.drupal.org/contributor-tasks/review

I applied the patch #31 but the result has a strange behavior which is not so user friendly.

Steps:

1) fresh 8.0.x install
2) create content view
3) add the "Content: Content author" relationship
4) add the field "User: Name"
5) delete the relationship

Result:
- the relationship has been removed
- the error message appears "You can not remove this relationship because it is used by other handlers (field: name)."

Problem:
- the message "You can not remove this relationship..." doesn't contains information about the removed relationship
- can't use the "Cancel" button in order to cancel the current changes (I got the same configuration + the error message mentioned above)

Tip:
- the solution would be more user friendly if the relationship could be deleted only if there are no other dependencies, in other case a validation message will popup

I think I performed the same steps, but got slightly different results.

After deleting the relationship and hitting apply, I got the message: You can not remove "uid" relationship because it is used by other handlers (field: name).
Cancel button works.

As @dawehner pointed out in #18, it would be better to check this at save. The number of scenarios that can lead to this (defaulted field vs overridden relationships, vice versa, multiple displays with different fields and relationships) makes checking this at run time pretty doomed I feel.
Also, when I'm editing my View I don't want to be bothered with "You can't do that" messages before I'm all the way done.

So, major overhaul. Just validation in the display at save. Lets see how this tests.

The core committers and Views maintainers (alexpott, xjm, effulgentsia, tim.plunkett, and dawehner) agreed that this is a major issue. A fatal is being caused under normal administrative site operation.

Looks reasonable for me

I noticed that I was unable to cancel after removing the relationship (as described in #37). Without the patch, I can cancel.

Is there some way to check if the Action is Cancel and skip validation?

For example:

In: core/lib/Drupal/Core/Form/FormBuilder.php
-      $this->formValidator->validateForm($form_id, $form, $form_state);
+      if ($form_state->getUserInput()['op'] != 'Cancel') {
+        $this->formValidator->validateForm($form_id, $form, $form_state);
+      }

Notice that without the patch Cancel brings you out of the edit and back to the views list.

@lokapujya I haven't tested this yet but 'Views validates displays on Cancel' sound like an issue unrelated to this patch. So if it is indeed a more generic issue, shouldn't we just open an issue for that and RTBC this one?

@lokapujya I haven't tested this yet but 'Views validates displays on Cancel' sound like an issue unrelated to this patch. So if it is indeed a more generic issue, shouldn't we just open an issue for that and RTBC this one?

+1

This patch solves a problem and doesn't introduce a new one.

OK, created a new issue: https://www.drupal.org/node/2671182

+++ b/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
@@ -2443,6 +2443,16 @@ public function validate() {
 
+    // Check for missing relationships.
+    $relationships = array_keys($this->getHandlers('relationship'));
+    foreach (ViewExecutable::getHandlerTypes() as $type => $handler_type_info) {
+      foreach ($this->getHandlers($type) as $handler_id => $handler) {
+        if (!empty($handler->options['relationship']) && $handler->options['relationship'] != 'none' && !in_array($handler->options['relationship'], $relationships)) {
+          $errors[] = $this->t('The %handler_type %handler uses a relationship that has been removed.', array('%handler_type' => $handler_type_info['lstitle'], '%handler' => $handler->adminLabel()));
+        }
+      }
+    }
+

This validates when a relationship has been removed and the view is saved.

But could we not validate when the relationship itself is removed or at least warn then? When you get here it's a bit late.

I can't decide whether it's more annoying to not be able to remove the relationship or not be able to save the view, but feels like the latter is probably more annoying. Obviously both are less annoying than fatal errors so if that would be much harder to implement could move to a follow-up - we already have the validation meta.

@dawehner argued against that in #18 and me in #39

I personally feel that not being able to remove the relationship is more annoying because it forces a workflow on me (remove all references to relationship first then remove relationship), where I feel that its a good thing that the UI doesn't bother me with errors while I'm mucking about until I tell it I'm actually done with all my changes, but that might just be me.

@Lendude
this reverts a test fix which wouldn't pass one of the checks you removed in #39

diff --git a/core/modules/views/src/Tests/Handler/HandlerTest.php b/core/modules/views/src/Tests/Handler/HandlerTest.php
index ae61c7b..6507e23 100644
--- a/core/modules/views/src/Tests/Handler/HandlerTest.php
+++ b/core/modules/views/src/Tests/Handler/HandlerTest.php
@@ -28,7 +28,7 @@ class HandlerTest extends ViewTestBase {
    *
    * @var array
    */
-  public static $testViews = array('test_view', 'test_view_handler_weight', 'test_handler_relationships', 'test_handler_test_access', 'test_filter_in_operator_ui', 'test_exposed_relationship_admin_ui');
+  public static $testViews = array('test_view', 'test_view_handler_weight', 'test_handler_relationships', 'test_handler_test_access', 'test_filter_in_operator_ui');

@kpv sorry, I don't follow. The test that was in HandlerTest is basically the same as the one that is now in DisplayTest. Not sure what you are referring to.

But could we not validate when the relationship itself is removed or at least warn then?

Validating would force the site builder to remove the fields before removing the relationship. Validating on save doesn't force a workflow.

How would a warning work? Maybe this discussion should happen in the views validation meta?

this reverts a test fix which wouldn't pass one of the checks you removed in #39

Doesn't that test only make sense when validating the relationship, but not when validating on save.

I tested this using Simplytest.me on a chrome browser using the patch in #39

Steps followed
1. Created a content type and added a content
2. Created a view and added fields from new content type
3. Created a relationship author
4. Added another field to the view User: Name
Screenshot here

5. Now deleted the relationship by clicking Rearrange->Remove
6. In Preview section, the warning showed up that "The field User: Name uses a relationship that has been removed. "
7. On clicking Save, got the error "The field User: Name uses a relationship that has been removed."
Screenshot here

8. I was able to cancel the relationship deletion at relationship screen and also edit view screen

Thanks @alexpott for the "ask-to-test"

In Preview section, the warning showed up that "The field User: Name uses a relationship that has been removed. "

That warning showing up is enough to cover my concern from #48, great that this already happens when the relationship gets removed.

Committed/pushed to 8.1.x and cherry-picked to 8.0.x. Thanks!

I added issue credit for dawehner, swetashahi and lokapujya.