Duplicating a Page to Block after changing the display id and then saving the view destroys the Page

Sounds a bit like #2897576: Resaving a view display results in deletion of view display - although not entirely the same

Nice find, nice tests, but I agree with @vaplas: not too sure about the fix.

Also, we seem to be fixing 2 bugs in one issue at the moment. If we can get one fix that fixes both cases I'm fine with doing it in one issue, but currently it's two fixes for two problems in one issue.

+++ b/core/modules/views/src/Entity/View.php
@@ -245,6 +245,7 @@ public function duplicateDisplayAsType($old_display_id, $new_display_type) {
+    unset($display_duplicate['new_id']);

+++ b/core/modules/views_ui/tests/src/Functional/DisplayCRUDTest.php
@@ -143,6 +143,27 @@ public function testDuplicateDisplay() {
+    // Duplicate display when changed id.
+    $view_id = $view->id();
+    $this->drupalPostForm("admin/structure/views/nojs/display/$view_id/page_2/display_id", ['display_id' => 'page_new'], 'Apply');
+    $this->drupalPostForm(NULL, [], 'Duplicate as Block');
+    $this->drupalPostForm(NULL, [], t('Save'));
+    $view = Views::getView($view_id);
+    $view->initDisplay();
+    $this->assertNotNull($view->displayHandlers->get('page_new'), 'The original display is saved with a changed id');
+    $this->assertNotNull($view->displayHandlers->get('block_2'), 'The duplicate display is saved with new id');

I think this should be a fix by itself. I think that is RTBC with just a nit fixed for the comment. And even if we come up with a generic way of preventing duplicate display names, just cleaning up the data when cloning makes sense anyway.

The other fix:

+++ b/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
@@ -1873,8 +1873,13 @@ public function validateOptionsForm(&$form, FormStateInterface $form_state) {
+            if ($id !== $this->view->current_display) {
+              if ($form_state->getValue('display_id') === $id ||
+                (isset($display->new_id) && $form_state->getValue('display_id') === $display->new_id) ||
+                (isset($display->display['new_id']) && $form_state->getValue('display_id') === $display->display['new_id'])
+              ) {

This seems very fragile. And I agree with @vaplas that it would be better to fix that whole logic at a different level then the UI. Some way we can just say 'We never allow duplicate display machine names when saving a view'. @vaplas suggestion of views/View::preSave sounds like a good place to start.

nitpicking some comments:

1. +++ b/core/modules/views_ui/tests/src/Functional/DisplayCRUDTest.php
   @@ -143,6 +143,27 @@ public function testDuplicateDisplay() {
   +    // Duplicate display when changed id.
   

   Test duplicating a display after changing the machine name.

2. +++ b/core/modules/views_ui/tests/src/Functional/DisplayCRUDTest.php
   @@ -143,6 +143,27 @@ public function testDuplicateDisplay() {
   +    // Prevent set display id, if other display also set same new id.
   

   Test setting the same machine name for two different displays before saving.

@vaplas thanks for that! This looks good.

Bumping this to critical since it leads to data loss through normal use of the UI.

Since it's not a common scenario, it might just be major, but it's not like it takes a weird setup to get into this state.

Committed/pushed to 8.6.x and cherry-picked to 8.5.x. Thanks!