PHP Warnings in views_preprocess_page() caused by frontpage view

I will take a look at this and see if I can produce an initial patch over this weekend.

Here is an initial patch file, I will try to add a test.

Views UI module is needs to be enabled in order to reproduce the warnings.

Updating previous patch to change array_key_exists() for an isset() as array_key_exists() would return TRUE for a NULL value of the key.

hi,
i am not able to reproduce this issue
i have followed following steps
Install minimal profile
Enable views
Go to /node whilst logged in as user 1
installed branch 8.x

please let me know i have missed any steps

Hello harshil.maradiya ,

The views_ui module needs to be enabled as well for the error to show.

Thank you stuart!

Test added to patch

@apexpott Here you go :)

I hope it's okay, I was unsure if I should be adding some kind of specific assert for the exceptions of if the system catching them is the expected/desired behaviour.

Currently waiting on https://drupal.org/node/2194703 as test is to be added to class mentioned here.

Attached is patch file containing only the test for this.

Automated test will fail as it should to show that previous patch must be applied as well to correct the issue stated here.

Thanks @Stuart Miller! Since we have the fix as well it's good practice to post the test + fix (combined) patch as well in the same comment. Once we have that then the test-only patch usually only needs to be posted again if there are significant changes made to the test.

Here are some items that can be fixed up:

1. +++ b/core/modules/node/lib/Drupal/node/Tests/Views/FrontPageTest.php
   @@ -149,4 +149,18 @@ protected function assertNotInResultSet(ViewExecutable $view, array $not_expecte
   +   * Tests the frontpage when logged in as admin
   

   This should be a complete sentence (end in a period) per https://drupal.org/node/1354#drupal.

2. +++ b/core/modules/node/lib/Drupal/node/Tests/Views/FrontPageTest.php
   @@ -149,4 +149,18 @@ protected function assertNotInResultSet(ViewExecutable $view, array $not_expecte
   +  protected function testAdminFrontPage() {
   

   Should be public.

3. +++ b/core/modules/node/lib/Drupal/node/Tests/Views/FrontPageTest.php
   @@ -149,4 +149,18 @@ protected function assertNotInResultSet(ViewExecutable $view, array $not_expecte
   +    // When user with sufficient permissions is logged in,
   +    // views_ui adds contextual links to the homepage view.
   +    // This verifies no errors are caused.
   

   The comments are wrapped really conservatively - they should be as long as possible within 80 characters (same reference as above).

Thanks for you comments, I have amended the patch accordingly.

Both patch and test in same patch.

Looks great. Test passes and fails appropriately and fix looks good. Great work @Stuart Miller!

Great work @Stuart. Fixes works fine for me as well!!

+++ b/core/modules/node/lib/Drupal/node/Tests/Views/FrontPageTest.php
@@ -149,4 +149,17 @@ protected function assertNotInResultSet(ViewExecutable $view, array $not_expecte
+    $this->drupalGet('node');

This makes sure no exceptions are thrown etc.. but I think we should assert that we can still see the view too? as you could have an empty body from that response and it would till pass.

So at the very least, a $this->assertResponse(200)?

I have added in an assert for the response code as well as one to check that the view appears on the page via a class name.

Thanks for pointing that out.

Thanks! Please post an interdiff when updating patches :)

Minor docs/typo cleanup, leaving at 'Needs review' but these are easy fixes:

1. +++ b/core/modules/node/lib/Drupal/node/Tests/Views/FrontPageTest.php
   @@ -149,4 +149,21 @@ protected function assertNotInResultSet(ViewExecutable $view, array $not_expecte
   +    // When user with sufficient permissions is logged in, views_ui adds
   +    // contextual links to the homepage view. This verifies there are no errors.
   

   This is a bit terse, how about "When a user"?

2. +++ b/core/modules/node/lib/Drupal/node/Tests/Views/FrontPageTest.php
   @@ -149,4 +149,21 @@ protected function assertNotInResultSet(ViewExecutable $view, array $not_expecte
   +    // Check that correct response was recieved from last request.
   

   Again a bit terse and there's a typo on 'received'. How about something like:

   Check that the correct response was received from the last request.

   Having said that, I'd also suggest that we don't need to comment every single line of this test method. I think this one in particular could definitely do without a comment :)

3. +++ b/core/modules/node/lib/Drupal/node/Tests/Views/FrontPageTest.php
   @@ -149,4 +149,21 @@ protected function assertNotInResultSet(ViewExecutable $view, array $not_expecte
   +    // Check that fontpage view was rendered.
   

   fontpage should be frontpage, and similar to the above comments maybe we can add a 'the' between Check and frontpage :)

Sorry about those typos, that's pretty sloppy!

I have amended and this time attached an interdiff from the previous patch #19

Thanks

Thanks @Stuart Miller, that looks good to me.

Committed and pushed to 8.x. Thanks!