Deleting Views Pages with Attachments Causes Fatal Error

Hi

I have followed the above steps but could not produce the error as mentioned in post. Here is further details

Drupal Version: 8.25(fresh)
PHP: PHP 5.6.27

Hi

Now I have produced the error but little bit different steps:

1. Create a view with a page. Let's just name it Views Page Test (views_page_test).
2. Save the view.
3. Create an attachment in the view. Attach the attachment to the Views Page that we just created.
4. Delete the page.
5. Save the view.
6. Click on the Attachments tab in the Views UI.
7. Notice that there's a warning saying "Plugin ID 'page_1' was not found."
8. Save the views two to three times
9. You will see error *The website encountered an unexpected error. Please try again later.*
10. The Attach To setting will show "Broken Handler". Clicking on it will do nothing. The edit panel will not come up because it's causing a fatal error.

Hi

I have fixed the issue on local and created the patch. Please try with the attached patch. Thanks

Based on the patch this should be the component.

Hi @cilefen

Sorry I did not get your point *Based on the patch this should be the component.* ?.

Hi @cilefen gotcha. No clarification needed. Thanks

The patch looks pretty solid, but we need to write some tests that's for sure :)
In general it would be great to expand the documentation to something, which explains WHY we do something specific.

Hi @dawehner

Do you want I add more details in code comments ?.

Hi

I have updated the previous patch. I added the following changes in new one.

1- Extended the previous comments
2- Corrected the variable name from $display_hander to $display_handler.

Hi

New patch contains the following things

1- Modified the comment
2- Added the Test code.

Thank you @asghar!

1. +++ b/core/modules/views_ui/src/ViewEditForm.php
   @@ -259,6 +259,16 @@ public function save(array $form, FormStateInterface $form_state) {
   +        if($attachments = $display_handler->getAttachedDisplays()) {
   

   Note: getAttachedDisplays returns an array, so we can skip the if here.

2. +++ b/core/modules/views_ui/src/ViewEditForm.php
   @@ -259,6 +259,16 @@ public function save(array $form, FormStateInterface $form_state) {
   +          foreach($attachments as $attachment ) {
   

   Nitpick: The CS requires a space after the foreach

Hi @dawehner

Thanks for your comment.
1-

FormStateInterface $form_state) {
+        if($attachments = $display_handler->getAttachedDisplays()) {

Yes return the array but I am also assigning the return output to the $attachments which I am using in *if block* condition. If deleting view display is not attached to the any attachment code then condition will be false.

2- I update that. Thanks

Yeah looking really nice! Just a little more nitpicking:

1. +++ b/core/modules/views_ui/src/Tests/DisplayAttachmentTest.php
   @@ -58,4 +58,37 @@ public function testAttachmentUI() {
   +   * Test the attachment working fine after the attached page deleted.
   

   Tests the attachment working after the attached page was deleted.

2. +++ b/core/modules/views_ui/src/Tests/DisplayAttachmentTest.php
   @@ -58,4 +58,37 @@ public function testAttachmentUI() {
   +  public function testAttachmentCrashFixed() {
   

   can we rename this to something describing the test? Like testRemoveAttachedDisplay or something along those lines?

3. +++ b/core/modules/views_ui/src/Tests/DisplayAttachmentTest.php
   @@ -58,4 +58,37 @@ public function testAttachmentUI() {
   +    $this->assertFieldById('edit-displays-settings-settings-content-tab-content-details-top-actions-delete', 'Delete Page', 'Make sure there is a delete button on the page display.');
   

   Not needed is it? We are not doing anything with the delete button at this point.

4. +++ b/core/modules/views_ui/src/Tests/DisplayAttachmentTest.php
   @@ -58,4 +58,37 @@ public function testAttachmentUI() {
   +
   +    // Save the attachment two times more to make sure attachment working fine.
   +    $this->drupalPostForm(NULL, array(), t('Save'));
   +    $this->drupalPostForm(NULL, array(), t('Save'));
   

   It this really needed? The test fails fine without the extra saved, so seems a little superfluous.

5. +++ b/core/modules/views_ui/src/ViewEditForm.php
   @@ -259,6 +259,16 @@ public function save(array $form, FormStateInterface $form_state) {
   +        // Remove view display from view attachment under the attachment options.
   

   Over 80 characters.

Hi @Lendude

Thanks for your comment. You can find my feedback against each point

1- Done
2- Done
3- Done
4- Yes at-least two times save needed for test. If we use one time then test pass and we cannot produce the error.
5- Done

I have attached the new patch file. Thanks

@asghar thanks for the rerolls, almost there I think, just a couple of things left for me:

1. +++ b/core/modules/views_ui/src/Tests/DisplayAttachmentTest.php
   @@ -58,4 +58,36 @@ public function testAttachmentUI() {
   +    // Save the attachment two times more to make sure attachment working fine.
   +    $this->drupalPostForm(NULL, array(), t('Save'));
   +    $this->drupalPostForm(NULL, array(), t('Save'));
   
   +++ b/core/modules/views_ui/src/ViewEditForm.php
   @@ -259,6 +259,16 @@ public function save(array $form, FormStateInterface $form_state) {
   +        // options. For more details visit https://www.drupal.org/node/2843787.
   

   Instead of saving X number of times and waiting for a fatal to show up, why not just test for the error that is shown?
   Why not take out the extra saves and replace it with:
   $this->assertNoText("Plugin ID 'page_1' was not found.");
   Then we are actually testing for this specific bug.

2. +++ b/core/modules/views_ui/src/ViewEditForm.php
   @@ -259,6 +259,16 @@ public function save(array $form, FormStateInterface $form_state) {
   +        // Remove view display from view attachment under the attachment ¶
   

   trailing whitespace snuck in

Also, could you add an interdiff when rerolling? Makes reviewing much easier.

Hi @Lendude

Thanks for your help. I have modified the requested changes.

@asghar nice, one last nitpick in the new comment and then I think this is ready

+++ b/core/modules/views_ui/src/Tests/DisplayAttachmentTest.php
@@ -58,4 +58,35 @@ public function testAttachmentUI() {
+    // Check there is no plugin id not found wraning.

Typo in warning and missing 'if', maybe "Check that there is no warning for the removed page display.", or something like that.

Thanks much appreciated. I have updated the patch.

@asghar nice work, many thanks.

Thanks @Lendude, Try to find the next issue and fix that.

1. +++ b/core/modules/views_ui/src/Tests/DisplayAttachmentTest.php
   @@ -58,4 +58,35 @@ public function testAttachmentUI() {
   +    $this->drupalPostForm(NULL, array(), t('Save'));
   

   There should be some kind of positive assertion after this, the next page could be a blank screen and assertNoText would still pass.

2. +++ b/core/modules/views_ui/src/ViewEditForm.php
   @@ -259,6 +259,16 @@ public function save(array $form, FormStateInterface $form_state) {
   +        // options. For more details visit https://www.drupal.org/node/2843787.
   

   This doesn't need the link back to the issue, git blame handles that OK.

Hi @catch

Thanks for guidance. I have updated the patch. Thanks

Nitpicks:

+++ b/core/modules/views_ui/src/Tests/DisplayAttachmentTest.php
@@ -87,6 +87,10 @@
+    ¶
+    // Check that current display is the attachment.

Whitespace got in.
And the comment doesn't match what the assertion does, maybe change it to something like 'Check that the attachment is no longer linked to the removed display.'

Thanks updated.

+++ b/core/modules/views_ui/src/Tests/DisplayAttachmentTest.php
@@ -87,6 +87,10 @@
+    ¶

This can be totally fixed on commit.

+++ b/core/modules/views_ui/src/Tests/DisplayAttachmentTest.php
@@ -58,4 +58,39 @@ public function testAttachmentUI() {
+    $this->assertText(t('Not defined'), 'The right text appears if there is no attachment selection yet.');

This is a good assertion to be used here!

Committed/pushed to 8.3.x and 8.4.x, thanks!

We are having this exact problem on 8.7.9

The views UI can not be accessed at all. Plus warning Plugin ID 'page_1' was not found. on admin pages.