Disabling feed view display does not remove feed header from display it is attached to

Deleting the 'Feed' display from the front page view *does* remove the rss header, though.

I think it still shouldn't be displayed when the display is disabled only. At the very least the feed url is an information leak, even if it does return a 404 when accessed.

Turns out this actually applies to all Feed views. The views display plugin attachTo() method is called, regardess of whether the display is enabled.

Attached patch checks whether a Feed display is enabled and does an early return if not. This stops rss headers being output on pages with disabled attached feed displays.

This probably needs a test.

Ah, can we also write some simple test coverage for it? This would be great

Added test coverage (I hope) in a new testDisabledFeed() function in DisplayFeedTest.php.

1. Enable the feed_display_test view
2. Attach the feed to the page display
3. Ensure the page display path returns status 200
4. Ensure the rss header is output when accessing the page display
5. Disable the feed display
6. Ensure the rss header is not output when accessing the page display
7. Ensure the feed display path returns status 404.

The full patch should pass testing, the testonly-mustfail patch should fail on the final two tests only.

Lets try that again then.

... and again. Weird unrelated test fails on local dev, so hopefully testbot will be more sensible. If not, I suspect there is an issue with the feeds test views; I was unable to make the feed_1 attachment display as attachment *at all* locally.

Okay, got further by adding a test view with attachment that in fact shows up locally, but I appear to not be able to edit that view to make the feed display be disabled without a schema validation error. Using the same code that ViewEditForm.php uses to set the enabled property to FALSE, but save() then errors out.

Not sure how to get around that. I am also not able to use $PluginTestsBase->container->get('entity.manager')->getStorage('view')->load('test_disabled_feed'); to load the view with pre-disabled display as exported from CMI.

Suggestions?

Spoke with larowlan. He reckons this is a schema problem, so this issue is blocked until #2410539: Views schema prevents saving of view with disabled display through code is resolved.

#20 was missing view.

jibran++
larowlan--

http://img2.wikia.nocookie.net/__cb20130617055401/thehungergames/images/...

Here is test-only.patch.

Manually tested the patch and confirmed that the RSS link was removed when I disabled the frontpage feed display. Screenshots attached.

I think this is RTBC, but I'm not sure about the views schema issue. Is that no longer blocking this issue with patch #22?

I just merged two patches so I think I can RTBC the issue. It is ready. Thanks @cafuego, @larowlan for the fixes, thank you @mradcliffe for the screenshots and thank you @kattekrab for reporting the issue.

Just checked, for other stuff we check for the isEnabled flag already, so +1 from me.

I think we should fix (and test) the missing enabled property in the schema first for all displays, then get this patch in.... rather than haphazardly fix enabled for only this single display type and hope it remains working by wya of a sort of unrelated test.

And #2410539: Views schema prevents saving of view with disabled display through code is green, if someone would care to have a look :-)

Okay, #2410539: Views schema prevents saving of view with disabled display through code is in, so this should now work without the extra schema change.

if green

Is this not a generic issue for all attachments - why is this not checked in Drupal\views\Plugin\views\display\Attachment::attachTo as well?

Should we be doing the enabled check here?

    // Find out which other displays attach to the current one.
    foreach ($this->display_handler->getAttachedDisplays() as $id) {
      $cloned_view = Views::executableFactory()->get($this->storage);
      $this->displayHandlers->get($id)->attachTo($cloned_view, $this->current_display, $this->element);
    }

#36 does sound more convincing .

Let's try #36

Test failures:

Drupal\Tests\views\Unit\ViewExecutableUnitTest

Looks like a unit test is expecting something to be enabled?

Drupal\views\Tests\PluginInstanceTest

Fatal error: Cannot use Drupal\Component\Utility\Xss as Xss because the name is already in use in /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/views/src/Plugin/views/field/Field.php on line 11
FATAL Drupal\views\Tests\PluginInstanceTest: test runner returned a non-zero error code (255).

I'll have a look at the fail.

That was easy.

That was easy

Now for the tricky bit: more testing.

I hadn't initially done this for non-feed displays, as a disabled attachment display on an enabled page display resulted in ... problems.

So I guess we'll now need to modify the shipped test views to stick the attachment on a page, then check whether that keeps working. Same for a block, embed and entityreference display. Should we split that all out into a new test file?

Refactored a little bit so the patch applies again. Added in a test view for disabled feed and attachments. Added disabled attachment test in DisplayAttachmentTest.php.

D'oh. Forgot to change the feed test URL.

... and one more test fix (attachments is an array with multiple items now, use in_array()).

We can clean both the tests now.

1. +++ b/core/modules/views/src/Tests/Plugin/DisplayAttachmentTest.php
   @@ -45,4 +55,54 @@ protected function testAttachment() {
   +    $view = $this->container->get('entity.manager')->getStorage('view')->load('test_attached_disabled');
   +    $view->getExecutable()->setDisplay('page_1');
   

   We can use View::getView() here to get ViewExecutable. So we don't have to getExecutable() again and again.

2. +++ b/core/modules/views/src/Tests/Plugin/DisplayAttachmentTest.php
   @@ -45,4 +55,54 @@ protected function testAttachment() {
   +    $result = $this->xpath('//div[contains(@class, "view-content")]');
   ...
   +    $result = $this->xpath('//div[contains(@class, "attachment-before")]');
   ...
   +    $result = $this->xpath('//div[contains(@class, "attachment-after")]');
   ...
   +    $result = $this->xpath('//div[contains(@class, "view-content")]');
   ...
   +    $result = $this->xpath('//div[contains(@class, "attachment-before")]');
   ...
   +    $result = $this->xpath('//div[contains(@class, "view-content")]');
   ...
   +    $result = $this->xpath('//div[contains(@class, "attachment-after")]');
   

   We have CssSelector in drupal core now.

We can use View::getView() here to get ViewExecutable. So we don't have to getExecutable() again and again.

Done.

We have CssSelector in drupal core now.

That's a cool story, bro.

We have plenty of tests now. I think it is good to go. Thanks @cafuego for fixes and tests.

Dear bot, please assess.

Refactored the test views, thanks Jibran.

Nice work @cafuego we have a unit tests and webtests for the change. I think it's good to go.

Added BE to IS.

Committed 37fec7e and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

diff --git a/core/modules/views/src/Tests/Plugin/DisplayAttachmentTest.php b/core/modules/views/src/Tests/Plugin/DisplayAttachmentTest.php
index 4eb43e6..add8491 100644
--- a/core/modules/views/src/Tests/Plugin/DisplayAttachmentTest.php
+++ b/core/modules/views/src/Tests/Plugin/DisplayAttachmentTest.php
@@ -62,7 +62,7 @@ public function testAttachment() {
    */
   public function testDisabledAttachments() {
     $this->drupalCreateContentType(['type' => 'page']);
-    $node = $this->drupalCreateNode();
+    $this->drupalCreateNode();
 
     // Ensure that the feed_1 display is attached to the page_1 display.
     $view = Views::getView('test_attached_disabled');
diff --git a/core/modules/views/src/Tests/Plugin/DisplayFeedTest.php b/core/modules/views/src/Tests/Plugin/DisplayFeedTest.php
index f48385d..512541e 100644
--- a/core/modules/views/src/Tests/Plugin/DisplayFeedTest.php
+++ b/core/modules/views/src/Tests/Plugin/DisplayFeedTest.php
@@ -81,7 +81,7 @@ public function testFeedOutput() {
    */
   public function testDisabledFeed() {
     $this->drupalCreateContentType(['type' => 'page']);
-    $node = $this->drupalCreateNode();
+    $this->drupalCreateNode();
 
     // Ensure that the feed_1 display is attached to the page_1 display.
     $view = Views::getView('test_attached_disabled');
diff --git a/core/modules/views/src/ViewExecutable.php b/core/modules/views/src/ViewExecutable.php
index 1c43095..1ccd58e 100644
--- a/core/modules/views/src/ViewExecutable.php
+++ b/core/modules/views/src/ViewExecutable.php
@@ -1579,7 +1579,7 @@ public function attachDisplays() {
     // Find out which other displays attach to the current one.
     foreach ($this->display_handler->getAttachedDisplays() as $id) {
       $display_handler = $this->displayHandlers->get($id);
-      // Only attach enable attachments.
+      // Only attach enabled attachments.
       if ($display_handler->isEnabled()) {
         $cloned_view = Views::executableFactory()->get($this->storage);
         $display_handler->attachTo($cloned_view, $this->current_display, $this->element);

Fixed on commit - unused variables and English.

Woop. A decade old feature request is fixed. :-)