node_help() broken for node add/edit form

I'll have a look at this issue now.

This patch should fix the problem. It does not include any tests.

Well... I'm a bit confused. The first time I tested without the second parameter it resulted in an exception. Then I added that parameter (since in some other frameworks the second parameter of a class load function selects the field that is used for selection) and it worked. :-)

I've now retested without the second parameter and it works well now. The source of the exception must have been something else.

Missing help text that is added by a user needs testing.

Gonna try writing a test.

Maybe we should decouple bug fixing and writing tests. In my opinion a missing test is a separate issue. Isn't it?

I'll contact firfin about the status of the test.

No reaction from firfin. I'll try to write a test.

No, tests should not be separate issues. While we're not quite doing test-driven development here, generally it is best and easiest to write the test when you're fully "up" on exactly what is supposed to happen here. If someone else writes the test later, months down the road perhaps, they may not realize that the code is not in fact doing what it is supposed to be doing, and basically write a test to confirm that the broken code continues to be broken.

I've written a test but it keeps failing. I guess the problem is that the help text is not added to the content type that is created by the test. Anybody a clue about how to do this in a test?

In the patch the fix and the test I've written.

I've done some more research and I've found that the help is in fact added to the content type config. So now my guess is that the problem is in activating the Bartik theme.

I've changed the test to use the standard profile instead of the testing profile and now it is working. But because it uses that standard profile it is quite slow so I don't know whether I've taken the best approach.

I've tried to enable Bartik in the test with the testing profile (installed it and set it as default) but the forms where still showing in the Classy theme. Since the test with the standard profile is working I leave it this way for the moment. If there are any suggestions to improve the test I'm happy to hear and learn.

Maybe the patch will be reviewed sooner when I remove the assignment.

I've refactored this a bit and made it work using the 'testing' profile. I have only limited experience writing tests for Drupal and so this was a bit of a learning experience, but I believe it works and I believe the refactoring is appropriate. Someone who knows better can tell us otherwise later. ;)

The changes I've suggested include the following.

First, setting up the content type has been moved to setUp() since we're not actually testing 'creation of a node type' or 'creation of a node type with help text' -- only that the help text is ultimately displayed if a block is available to display it. Logically, this is in the nature of 'setup' and not 'testing' as a result. (This necessitated creating the $testType and $testText properties.)

Second, $adminUser need not be a class property as it is never used outside of setUp(). I know that many of the tests set $this->adminUser and $this->webUser, but in many cases there seems to be no point to doing so and I don't think we should continue the practice 'just because', unless someone can tell me a good reason for doing so.

Testbot should be happy with this, but let's see what other reviewers think, too.

Ahem. Diff'd and submitted from one revision too early, before I fixed up the comments.

In my opinion the suggested changes are fair. Little remark about coding standards:

+    $this->drupalLogin ($admin_user);
+    $this->drupalPlaceBlock ('system_help_block');

I don't think there should be spaces after the method names.

I'll try to test the re-factored patch later today.

You're right on the coding standards issue. I'll wait on fixing that until your testing is complete in case something else needs tweaking besides that ditty.

The re-factored patch is working. When the spaces mentioned in comment #20 are removed we can make the patch RTBC.

Setting to review to get the testbot on board for a final check before RTBC, but the changes in this patch are minimal.

diff --git a/core/modules/node/src/Tests/NodeShowHelpTextTest.php b/core/modules/node/src/Tests/NodeShowHelpTextTest.php
index 8e66543..f33b180 100644
--- a/core/modules/node/src/Tests/NodeShowHelpTextTest.php
+++ b/core/modules/node/src/Tests/NodeShowHelpTextTest.php
@@ -50,8 +50,8 @@ protected function setUp() {
       'bypass node access',
     ));

-    $this->drupalLogin ($admin_user);
-    $this->drupalPlaceBlock ('system_help_block');
+    $this->drupalLogin($admin_user);
+    $this->drupalPlaceBlock('system_help_block');

     $this->testType = 'type';
     $this->testText = t('Help text to find on node forms.');

The class-level description is sufficient to document "what it does" in any event, and duplicating the same for that function isn't likely to add anything IMO. Suggested revision (not submitting a patch in case there is vehement disagreement on this point):

diff --git a/core/modules/node/src/Tests/NodeShowHelpTextTest.php b/core/modules/node/src/Tests/NodeShowHelpTextTest.php
index f33b180..59dc436 100644
--- a/core/modules/node/src/Tests/NodeShowHelpTextTest.php
+++ b/core/modules/node/src/Tests/NodeShowHelpTextTest.php
@@ -10,7 +10,7 @@
 use Drupal\simpletest\WebTestBase;

 /**
- * Verify that a help text is shown on node forms.
+ * Verify that content-type specific help text appears on node add/edit forms.
  *
  * @group node
  */
@@ -63,10 +63,6 @@ protected function setUp() {
     ));
   }

-  /**
-   * Verifies that the 'testing' help text appears on the "node add" and
-   * "node edit" forms for the test content type.
-   */
   public function testNodeShowHelpText() {

Hope this one satisfies everybody! :)

Ok... I did that previous one to quickly...

Looks good to me.

If you are confident that the fix and the test in the patch are working and code and comments are according to the coding standards you can set the issue to RTBC.

1. +++ b/core/modules/node/node.module
   @@ -121,7 +121,8 @@ function node_help($route_name, RouteMatchInterface $route_match) {
   -      $type = $node->getType();
   +      $type_name = $node->getType();
   +      $type = NodeType::load($type_name);
   

   This could be just $type = NodeType::load($node->getType()); - no need for the extra variable here.

2. +++ b/core/modules/node/src/Tests/NodeHelpTest.php
   @@ -0,0 +1,81 @@
   +    $this->assertText($this->testText, 'Help text is found on node add form.');
   ...
   +    $this->assertText($this->testText, 'Help text is found on node edit form.');
   

   No need to provide the 'Help text is found on node edit form.' message - the default assertion message is actually more useful to a developer when this test fails.

Revised patch attached.

Patch is working as expected. Changes as suggested by alexpott are included.

This issue is a normal bug fix, and doesn't include any disruptive changes, so it is allowed per https://www.drupal.org/core/beta-changes. Committed 02c173e and pushed to 8.0.x. Thanks!