Journal entry does not save at admin/build/block

Possibly, this is related to #479358: Journal entry does not save at admin/content/node-type/[content-type-name]/display

Reassigning back to the proper queue -- see http://drupal.org/node/26160/revisions (latest revision) for more on this.

oh. Wild guess:

  $form = array(
    '#action' => arg(4) ? url('admin/build/block/list/'. $theme_key) : url('admin/build/block'),
    '#tree' => TRUE,
  );

This means that all form element keys are hierarchical. Journal expects its values without #tree. Most probably, this can be solved by manually forcing #parents for Journal's form additions - or setting #tree to FALSE for Journal's parent element keys (not sure).

Here is the fix that helped me:

$form['journal'] = array(
    '#weight' => $journal_weight,
    '#tree' => FALSE,
  );

(just add '#tree' => FALSE to the code in line 123-124)

That, as a proper patch, is almost RTBC. :)

This patch includes the fix from sun and restyler above. It also includes the begining of journal.test which tests this bug as well as the basic functionality of the module. This patch was created against 6.x-1.x-dev and can be applied with "cd journal; patch -p0 474422.patch;"

Actually please use this patch as it includes a fixed copy of the journal.test file that also has a test for issue 479358. It can be applied as above: "cd journal; patch -p0 474422_479358.patch"

Thanks! Looks promising from a quick peek.

See http://drupal.org/node/156119 about issue statuses.

Since it seems that you fine folks want to provide some other patches, too - two ways to go from here:

a) I'll review this patch in detail and you fix the coding-style according to Drupal core's coding-standards. (Though I should note that I'm one of Drupal's chief-nitpickers.)

b) I'll fix a) myself before committing. Though that will most probably mean that this patch will languish a bit here until I can spare some time.

I am not sure if you were insinuating that I needed to run a check on the code by the Drupal coding standards, so I did just in case. Here is a patch fixing the feedback I got from the coder module (including some misc fixes to .module and .install). It can be applied as above with (cd journal; patch -p 0 < 474422_479358_coder.patch). It should be noted that coder complains about camel cased functions in the test module, but this seems to be the standard across all other test files. A number of the parent methods of the test module in simpletest are in fact camel cased.

If this fact goes against your ideas of right and wrong coding practices, be sure to let me know and I will be happy to name them as you specify.

Holy cow! Awesome.

Yes, Journal module has come a long way, and certainly, I didn't adhere to Drupal's coding standards when I started to code for Drupal. :)

So here is the review:

+    #bug fix for issue 474422

Please replace that comment, using proper syntax for inline comments, always explaining why something is done, for example:

     // Reset #tree to get form values where we expect them.

...

+ * Simpletest integration for theJournal module

Missing space.

+  /** this array will store that blocks that we add so they can later be 
+   * remoced in the tearDown() function
+   */

PHPDoc comments never start in the first line. Also a typo in that comment ("remoced").

+  /** Required simpletest methods */
...
+  /** Unit tests */
...

Please remove those "chapter" comments.

+  /**
+   * Implementation of getInfo().
+   */
+  public static function getInfo() {

getInfo() is the only exception we do not document. So just remove that PHPDoc, please.

+      // 'name' should start with what is being tested (menu item) followed by what
+      // about it is being tested (creation/deletion).
+     'name' => t('Journal module functionality'),
+
+      // 'description' should be one or more complete sentences
+      // explaining the test.
+      'description' => t('Test the journal module functionality.'),
+
+      // 'group' should be a logical grouping of test cases, like a category.
+      // Suggestion: Use the name of the module to be tested.
+      'group' => t('Journal'),

We do not document SimpleTest basics, because documentation exists elsewhere. All of these comments need to be removed. Also note that those comments do not wrap at 80 chars.

+      0 => 'administer blocks',
+      1 => 'access journal',
+      2 => 'administer content types',

Array keys are superfluous here.

+  /** TODO: if testJournal479358 succeeded we need to put the block back */

Proper PHPDoc syntax for todos is @todo, followed by a space and the note. If the note is longer than 80 chars, following lines should use an extra indentation of 2 spaces.

+  /** TODO: if testJournal479358 succeeded we need to put the block back */
+  function tearDown() {
+    parent::tearDown();
+
+    #foreach ($block as $blocks) {
+    #}
+  }

That said, this method does nothing, so we remove it. If the block is needed for another test, we implement a new testcase.

+  }
+
+
+  /** Unit tests */

Doubled new line here. One is sufficient. Regarding that "chapter" comment, see above.

+  /** 
+   * Tests a basic journal entry
+   */

PHPDoc summaries should use the first form (not the third, i.e. without trailing "s"). All comments should form proper sentences, i.e. using a trailing period (full-stop).

+    /** Add a new block */

Wrong syntax for inline comments. We use // This is a comment.

+    $blocks[] = $title;

$blocks is not defined; this leads to a PHP notice, because the value is assigned to a new array item without using a key.

+    $this->drupalPost('admin/build/block/add', $edit, 'Save block');
+    $this->assertText($description, t('Verify that the block was added'));
+    $this->drupalGet(url('admin/reports/journal', array("absolute" => TRUE)));
+    $handle = fopen('/tmp/journaltest.html', 'w');
+    fwrite($handle, $this->drupalGetContent());
+    fclose($handle);
+    $this->assertText($journal_entry, t('Verify that the journal entry is listed'));

We want to add some comments about what is done here. Also, it looks like some debugging code was left over.

+  /**
+   * Test of a bug that would not allow journal to add entries with cck enabled
+   * at url: admin/content/node-type/[content-type-name]/display
+   *
+   * Issue: http://drupal.org/node/479358
+   * create a new text type
+   * add it to the story type
+   * do to admin/content/node-type/story/display
+   * submit with a journal and make sure that it exists
+   */

If you (basically) negate the summary, so it summarizes the test, then the summary would work. We do not refer to bugs in PHPDoc. Note that PHPDoc summaries (the first line) never wraps (only the description should wrap at 80 chars). That additional PHPDoc description should be injected using inline comments into appropriate places in the test. So the remaining PHPDoc will (basically) be:

+  /**
+   * Test that a journal entry is recorded when form is using #tree = TRUE.
+   */

...

+    parent::setUp('journal', 'content', 'text');
...
+  function testJournal479358() {
...
+  function testJournal474422() {

Both tests test the same (see above, revised PHPDoc summary), so we only need one. We want to keep the one for blocks, because Block module is not an optional contrib module.

The test method needs a proper name - based on above PHPDoc, I'd recommend 'testFormTreeEnabled'.

+    $edit = array(
+      'info' => $this->randomName(8),
+      'title' => $title,
+      'body' => $this->randomName(8),
+      'pages' => '',
+      'journal_entry' => $journal_entry,
+    );
+
+    $this->drupalPost('admin/build/block/add', $edit, 'Save block');

drupalPost should directly follow the $edit array, so the dependent code is also visually connected.

Please see attached the new patch with your recommendations.

Were your recomendations made from manually reviewing the code, or is there some additional tool I should be using for checking my code against drupal coding standards?

=== PATCH redacted (see below) ===

whoah ... not sure exactly what happened there. I don't think I should be submitting patches after workign outside all day, the previous one is not complete. Please use the one below.

@@ -130,6 +130,7 @@ function journal_form_alter(&$form, &$fo
   $form['journal']['journal_location'] = array(
     '#type' => 'hidden',
     '#value' => (!empty($_REQUEST['journal_location']) ? $_REQUEST['journal_location'] : $_GET['q']),
+    '#tree' => FALSE,
   );
 
   // Add journal entry field.
@@ -139,6 +140,7 @@ function journal_form_alter(&$form, &$fo
     '#description' => t('If not empty, contents of this field will be logged to the system journal.'),
     '#required' => $entry_required,
     '#wysiwyg' => FALSE,
+    '#tree' => FALSE,
   );
   if ($entry_required && user_access('access devel information')) {
     $form['journal']['journal_entry']['#required'] = FALSE;

Now I have to wonder: Contrary to what comment #6 in this issue is saying, you are applying #tree to all sub-elements instead of the parent 'journal' element - why is that?

+     'description' => t('Test the journal module functionality.'),

In this context, "journal" should be written as proper noun, i.e. properly capitalized.

+  function setUp() {
+    parent::setUp('journal', 'content', 'text');

I don't think that Content and Text modules are required for the test any longer.

+    $user = $this->drupalCreateUser(array(
+      'administer blocks',
+      'access journal',
+      'administer content types',
+    ));

Can we write that permissions array onto one line, please? The other $edit arrays can be left multi-line.

+  /** 
+   * Tests a basic journal entry
+   */

Not using first form ("Test" instead of "Tests") and missing punctuation, i.e. trailing period (full-stop). This applies to all PHPDoc summaries in the patch.

+    //Add a new block
...
+    //make sure the journal entry we added above is in the journal report

Inline comments should use a leading space, i.e. // Add... instead of //Add..., form a proper sentence (first letter uppercase), and use proper punctuation (trailing period).

+  function testJournalAddJournal() {

Rename to testJournalAddEntry() ?

+  function testJournalFormTreeEnabled() {
+
+    //Add a new block
+    $title = $this->randomName(8);
+    $journal_entry = $this->randomName(8);
+
+    $edit = array(
+      'info' => $this->randomName(8),
+      'title' => $title,
+      'body' => $this->randomName(8),
+      'pages' => '',
+      'journal_entry' => $journal_entry,
+    );
+    $this->drupalPost('admin/build/block/add', $edit, 'Save block');
+
+    //verify the entry is in the journal report
+    $this->drupalGet(url('admin/reports/journal', array("absolute" => TRUE)));
+    $this->assertText($journal_entry, t('Verify that the journal entry is listed'));
+    $this->assertText('admin/build/block/add', t('Verify that the journal entry location is listed'));

It looks like these assertions are the same as in testJournalAddJournal(), except the last assertText(). If so, we should strip this block and move the last assertion into testJournalAddJournal(), i.e. testJournalAddEntry().

+    $this->drupalGet(url('admin/reports/journal', array("absolute" => TRUE)));

Could you explain why you did not use $this->drupalGet('admin/reports/journal');? I am pretty sure that there is no reason, so we should use the standard way.

+  function testJournalFormTreeEnabled() {
+
...
+  }
+
+}

Please no leading or trailing blank lines in the body of classes and functions.

I am applying "'#tree' => FALSE,' to both journal_entry and journal_location because they are both affected by the same bug. I discovered this when adding in the logic for Allow single tags for Journal entries today.

Regarding testJournalAddEntry() vs. testJournalFormTreeEnabled() having similar functionality, it is a good point, but they are two logically different tests. From the perspective of test based coding there should be a test that exercises the basic functionality of a module (testJournalAddEntry()) and one for each bug that is found for regression testing (testJournalFormTreeEnabled()). While this increases the amount of code that exisits in the journal.test file I think it serves as good documentation. But then again since this is the first revision of journal.test, maybe this logic should go into the testJournalAddEntry() function. This is what I ended up doing. What are your thoughts on this?

Please see the attached patch.

I highly believe that restyler's solution in comment #6 should work equally, because #tree is applied recursively by Form API. That means, we do not have to apply #tree to each and every element below $form['journal'], but exactly that parent element only instead.

 /**
+ * @file
+ * journal.install for the journal module
+ */

...should form a proper sentence - ideally rephrase into "Installation functions for Journal module."

+/**
+ * @file
+ * Simpletest integration for the Journal module
+ */
+class JournalTestCase extends DrupalWebTestCase {

Same here, but also missing blank line between @file PHPDoc block and class. There should only be no blank line if the PHPDoc belongs to the class/function (which also means that the PHPDoc for this test case class is missing ;) ).

+    $this->assertText('admin/build/block/add', t('Verify that the journal entry location is listed'));
+
+    // Position the new block adding a journal entry.

If you split the test here, re-introducing the former testJournalFormTreeEnabled(), then we have two atomic tests - one "simple" that ensures basic functionality. And a first "special" test that ensures functionality under a certain condition (#tree).

+    $this->drupalGet(url('admin/reports/journal', array("absolute" => TRUE)));

Still using a special way for getting 'admin/reports/journal' without explanation and most probably without reason. (two times)

Note that marcp's second nitpick is basically correct (-- Always use single quotes whenever double quotes are not technically required). However, we want to complete remove that url() invocation in $this->drupalGet(), because it is superfluous -- at least I see no reason why it should be required.

please see attached with the changes you recommended

RE the comment in #17 and #21 about using a "complete sentence": the line

"Installation functions for Journal module."

is not a complete sentence, as there is nary a verb to be found.

I'm assuming that we want the following rules in place:

1. Start comments with a capital letter;
2. End comments with a period (as suggested by sun in #12); and
3. Capitalize the module name when it is referred to as a proper noun (ie, "the Journal module" as opposed to "save a journal entry".)

Does that capture what we're shooting for?

Please see attached patch as per #21

Also the line after @file is where the summary goes, following this another newline delimits an optional description.

PS. you two should really submit to the coder module, so that these checks could be done automatically by me and other contributers who had to take an extra semester of English in college.

heh. I'm not good at English either. But I wrote the coder_format script that ships with Coder module. ;)

This looks much, much better now!

Only remaining issues I quickly note only, since already explained in detail above:

1) Missing blank line between @file and class in journal.test. (Optionally add PHPDoc for the class, though I'm not eager on that).

2) Split testJournalAddEntry() into two tests, existing test for admin/build/block/add, separated one for admin/build/block.

3) Remove url() from $this->drupalGet().

Sun,

testJournalAddEntry() is the combination of two separate functions that you had earlier suggested be one function. In a previous function, the first check test a basic journal entry that did not touch the '#tree' => FALSE bug, the second function _did_ touch the '#tree' => FALSE bug. At your suggestion I joined them.

I have address issue #1 and #3, please see patch below.

Splitting testJournalAddEntry() in two tests;

#24

vs.

#17

I'm assuming that both posts are describing the same situation? If so I think it was a little unclear in #17 what that discussion was pertaining to since in #15 you had asked evoltech to combine into one function. If I understood the discussion properly, you are asking for testJournalAddEntry() to be split as described in #17.

If I'm mistaken please clarify.

fixes the drupalGet() (in journal.test) issue mentioned above.

Thanks! We are close to completion. :)

The confusion probably arose, because not all of my found issues were incorporated in the very next patch after each review. To clarify:

In #12 you had 3 tests:

- testJournalAddJournal()
- testJournal479358()
- testJournal474422()

and I asked to combine the second with the third.

After you did so, in #15 I then realized that

- the first part of the combined testJournalFormTreeEnabled() is the same as in testJournalAddJournal(), only with 1 additional assertion

and I asked to move that additional assertion into testJournalAddJournal() and remove the first part from testJournalFormTreeEnabled().

In #17, all tests were suddenly merged into one. ;)

#24 only repeated a few issues from #17 that were not yet incorporated.

Final issues:

- We sill want to have two atomic tests, testJournalAddEntry() for admin/build/block/add, and testJournalFormTreeEnabled() for admin/build/block (the #tree = TRUE test).

- Note that test methods use camelCase names like "testJournalAddEntry()", not "test_journal_add_entry()".

-      // Output delimiter in first line, since this may chance.
-      $output = '\t' . "\n";
-      
+      // Output delimiter in first line, since this may change.
+      $output = '\t'."\n";
+

The new white-space fix here is good, but the change in string concatenation is not (please revert).

fixed $output = '\t' . "\n"; issue

fixed

- We sill want to have two atomic tests, testJournalAddEntry() for admin/build/block/add, and testJournalFormTreeEnabled() for admin/build/block (the #tree = TRUE test).

issue

Please see attached patch

test methods use camelCase names like "testJournalAddEntry()", not "test_journal_add_entry()".

fixed

Tests always go into a sub-directory "tests".

Did the back-port to D5 myself. ;)

Thanks for reporting, reviewing, and testing! Committed to all branches.

A new development snapshot will be available within the next 12 hours. This improvement will be available in the next official release.

oh, and please do a diff between your version and the new committed code - I did some further optimizations here & there.