node_views_analyze() is never executed because it is in the wrong inc file

The patch no longer even came close to applying, but I think the issue is still valid. I've taken the liberty of attempting a re-roll.

The patch works for me.

there is not test coverage for this hook in the node module

I am not sure if this should be included here. This issue is just about wrong location of this hook. Since the hook implementation needs revising I propose we handle this in #2925477: Revise node_views_analyze message.

Thanks for working on this!

This issue is a bugfix, so it does need test coverage that proves the bug is fixed. We should add a test that fails against HEAD and passes when combined with the test that moves the hook implementation. Test coverage is a core gate, so we don't postpone it to followup issues even if the state of the test coverage in HEAD is not great.

So, setting "Needs work" to add automated test coverage. Thanks!

Will try to work on this test coverage.

Giving it a try on my first test coverage with #7patch together.
I'm not sure if the asserting method for analyze message is the best approach, if there are any better suggestion, I am all ears!

Setting to needs review.

adding bug smash initiative tag

@g-brodiei nice! Great first effort!

1. +++ b/core/modules/node/tests/src/Functional/Views/NodeViewsAnalyzeTest.php
   @@ -0,0 +1,88 @@
   +use Drupal\views\Views;
   

   Unused use

2. +++ b/core/modules/node/tests/src/Functional/Views/NodeViewsAnalyzeTest.php
   @@ -0,0 +1,88 @@
   + * Tests the bug fix for node_views_analyze hook.
   

   This should explain what the test tests, not reference why it was added. 'Tests node_views_analyze().' should be fine.

3. +++ b/core/modules/node/tests/src/Functional/Views/NodeViewsAnalyzeTest.php
   @@ -0,0 +1,88 @@
   +  ¶
   +  /**
   +   * Modules to be enabled.
   +   * ¶
   +   * @var array
   

   Lots of trailing white space in the test, that needs to go.

4. +++ b/core/modules/node/tests/src/Functional/Views/NodeViewsAnalyzeTest.php
   @@ -0,0 +1,88 @@
   +  public static $modules = ['views', 'views_ui', 'node_test_views'];
   

   this should be protected

5. +++ b/core/modules/node/tests/src/Functional/Views/NodeViewsAnalyzeTest.php
   @@ -0,0 +1,88 @@
   +  protected function setUp($import_test_views = TRUE) {
   

   on 9.1 this needs :void

6. +++ b/core/modules/node/tests/src/Functional/Views/NodeViewsAnalyzeTest.php
   @@ -0,0 +1,88 @@
   +    $this->drupalCreateContentType(['type' => 'page']);
   ...
   +    // Create two nodes.
   +    for ($i = 0; $i < 2; $i++) {
   

   Why are we creating nodes? Is that needed to analyse the View?

7. +++ b/core/modules/node/tests/src/Functional/Views/NodeViewsAnalyzeTest.php
   @@ -0,0 +1,88 @@
   +  public function testNodeViewsAnalyze() {
   

   Needs a docblock to describe that is being tested

8. +++ b/core/modules/node/tests/src/Functional/Views/NodeViewsAnalyzeTest.php
   @@ -0,0 +1,88 @@
   +    $this->assertSession();
   

   This doesn't do anything, we can take it out

9. +++ b/core/modules/node/tests/src/Functional/Views/NodeViewsAnalyzeTest.php
   @@ -0,0 +1,88 @@
   +    /**
   +     * Will SUCCESS if contains proper error message.
   +     * Will FAIL if bug patch not applied.
   

   No need to explain this, that is what assertions do (and we don't use /** comments only //)

10. +++ b/core/modules/node/tests/src/Functional/Views/NodeViewsAnalyzeTest.php
    @@ -0,0 +1,88 @@
    +     * @see node_views_analyze() at core/modules/node/node.views.inc.
    

    If the method has a docblock pointing to this, there is no need to mention it here

Oh and please add a test-only patch (with only the test, not the fix) so we can see that the fix actually works.

@Lendude, thank you for reviewing! Submitting a test to fail patch and fixed patch with suggestions.

For the suggestion, there is one I didn't change.
4. I can't run test if $module was set to protected, shown on the error message returned, so remaining as public.

Also removed useless override of function setUp() since there isn't any node needed to be created.

Add interdiff for review.

Setting back to needs review, the failing test was meant to fail as the patch wasn't combined.

This is looking pretty good, thanks for working on it. The $modules property changed from public to protected in 9.0.0. Patches should be produced for the latest version first and then backported if necessary, so bumping this to 9.1.x.

1. +++ b/core/modules/node/tests/src/Functional/Views/NodeViewsAnalyzeTest.php
   @@ -0,0 +1,49 @@
   +  /**
   +   * Modules to be enabled.
   +   *
   +   * @var array
   +   */
   

   This can just be

     /**
      * {@inheritdoc}
      */
   

2. +++ b/core/modules/node/tests/src/Functional/Views/NodeViewsAnalyzeTest.php
   @@ -0,0 +1,49 @@
   +  public static $modules = ['views_ui', 'node_test_views'];
   

   In Drupal 9 this needs to be protected, please see https://www.drupal.org/node/2909426

3. +++ b/core/modules/node/tests/src/Functional/Views/NodeViewsAnalyzeTest.php
   @@ -0,0 +1,49 @@
   +   * {@inheritDoc}
   

   inheritDoc -> inheritdoc

   (annoyingly, the PHPStorm autocomplete gets this wrong for Drupal!)

Added a patch with the above mentioned changes by #25.

Thank you Longwave for reviewing and Vidushi Mehta for creating the patch for 9.1!
Going to set this to RTBC!!

I don't think we should be recommending panels here - can we at least change the message to recommend layout builder? I know that introduces a bit of a conflict with #2925477: Revise node_views_analyze message.

Updated patch and interdiff.

+++ b/core/modules/node/node.views.inc
@@ -0,0 +1,52 @@
+        $ret[] = Analyzer::formatMessage(t('Display %display has set node/% as path. This will not produce what you want. If you want to have multiple versions of the node view, use layout builder.', ['%display' => $display->display['display_title']]), 'warning');

Sorry to nitpick but in the module list this is called Layout Builder, I think we should follow the same capitalisation?

I wondered about that but panels was lowercase so I followed that convention. Easy to change. Patch attached with title case, the committer can choose which one :)

Wow, the progress moved so fast on this one today! I forgot to reload the page and went generating patches after work just then, and it's done already when I reloaded the page LoL. AWESOME!!!!

Adding review credits

Fixed on commit:

diff --git a/core/modules/node/tests/src/Functional/Views/NodeViewsAnalyzeTest.php b/core/modules/node/tests/src/Functional/Views/NodeViewsAnalyzeTest.php
index cc94caa819..057d70cbfc 100644
--- a/core/modules/node/tests/src/Functional/Views/NodeViewsAnalyzeTest.php
+++ b/core/modules/node/tests/src/Functional/Views/NodeViewsAnalyzeTest.php
@@ -9,7 +9,7 @@
  */
 class NodeViewsAnalyzeTest extends NodeTestBase {

-   /**
+  /**
    * {@inheritdoc}
    */
   protected static $modules = ['views_ui', 'node_test_views'];

Committed db13a84 and pushed to 9.1.x. Thanks!

Moving to 9.0.x for a possible backport once freeze is over.

Setup an 8.9 test run

Sidenote: this was my 1000th commit to Drupal core 🎉

I believe this needs rerolled and working versions for both 8.9 and 9.0

Rerolled for both 8.9 and 9.0. Please Review.

Thanks for helping on rerolling ayushmishra206!

The $modules test property was made protected in D9, but should be public for D8.

as mentioned here by xjm 112.

+++ b/core/modules/node/tests/src/Functional/Views/NodeViewsAnalyzeTest.php
@@ -0,0 +1,47 @@
+   * {@inheritdoc}
+   */
+  protected static $modules = ['views_ui', 'node_test_views'];
+

should be made public for 8.9, and protected for 9.

Would love another set of eye to review too!

Reroll for 8.9.x, add interdiff for 39-43

Tried rerolling #31 patch for 9.1.x following the guide on reroll patch, no conflicts after rebasing on step 9, so I requested a retest on the #31 patch against 9.1.x.

Backported 186b0a3 and pushed to 9.0.x. Thanks!

Fixed on commit

diff --git a/core/modules/node/tests/src/Functional/Views/NodeViewsAnalyzeTest.php b/core/modules/node/tests/src/Functional/Views/NodeViewsAnalyzeTest.php
index cc94caa819..057d70cbfc 100644
--- a/core/modules/node/tests/src/Functional/Views/NodeViewsAnalyzeTest.php
+++ b/core/modules/node/tests/src/Functional/Views/NodeViewsAnalyzeTest.php
@@ -9,7 +9,7 @@
  */
 class NodeViewsAnalyzeTest extends NodeTestBase {

-   /**
+  /**
    * {@inheritdoc}
    */
   protected static $modules = ['views_ui', 'node_test_views'];

Backported to 8.9.x as well.