saveBookLink() in \Drupal\book\BookManager should return an array on success but doesn't return anything.
This causes submit() in \Drupal\book\Form\BookOutlineForm to think an error has occurred on saving the new book outline, even though it has been successful.

Screenshot of incorrect error on changing book outline

Patch returns the $link array as specified in BookManagerInterface.

CommentFileSizeAuthor
#12 BookManager-saveBookLink-return-array-11.patch1.53 KBmcjim
#9 BookManager-saveBookLink-return-array-9.patch1.36 KBmcjim
#8 BookManager-testSaveBookLink-do-not-test.patch1.17 KBmcjim
#6 Incorrect-error-message-2208325-6.patch943 bytesanksy
BookManager-saveBookLink-return-array.patch472 bytesmcjim
Screen Shot 2014-03-01 at 15.33.24.png38.55 KBmcjim
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mcjim’s picture

mcjim CreditAttribution: mcjim commented
Issue summary: View changes
mcjim’s picture

mcjim CreditAttribution: mcjim commented
Issue summary: View changes
mcjim’s picture

mcjim CreditAttribution: mcjim commented
Issue summary: View changes
dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Issue tags: +Needs tests

Beside from the potential unit test we should really ensure to test this functionality using a simple test, see the BookTest class.
If you need some help with that, just ping me somewhere.

Your fix is perfect.

lhangea’s picture

lhangea CreditAttribution: lhangea commented

#4 I'm a newbie in writing unit tests and I tried writing one for this issue. However I encounter dificulties because I want to unit test the method saveBookLink to see if it returns or not an array. But this method calls loadBookLink which connects to the database (which it cannot happen in unit testing as far as I know ). How am I supposed to unit test this ?

Thanks

anksy’s picture

anksy CreditAttribution: anksy commented
FileSize
Incorrect-error-message-2208325-6.patch943 bytes

This is the test for the issue.

Status: Needs review » Needs work

The last submitted patch, 6: Incorrect-error-message-2208325-6.patch, failed testing.

mcjim’s picture

mcjim CreditAttribution: mcjim commented
FileSize
BookManager-testSaveBookLink-do-not-test.patch1.17 KB

Attached is work-in-progress on a unit test for saveBookLink().
It's stalled, however, because we can't mock the database connection.

Migrate module is able to mock a database connection, and if that code is pulled into core, we could use that. See #2225473: Move database fakes out of migrate module so they can be used in non-migrate tests.

mcjim’s picture

mcjim CreditAttribution: mcjim commented
FileSize
BookManager-saveBookLink-return-array-9.patch1.36 KB

New patch that includes a simpletest test to check that saveBookLink returns something.

mcjim’s picture

mcjim CreditAttribution: mcjim commented
Status: Needs work » Needs review
dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

Cool!

+++ b/core/modules/book/lib/Drupal/book/Tests/BookTest.php
@@ -585,4 +585,21 @@ public function testBookOutline() {
+    // Test that something is returned.
+    $this->assertNotNull($return);

Can we test a little bit more, like the content of it?

mcjim’s picture

mcjim CreditAttribution: mcjim commented
FileSize
BookManager-saveBookLink-return-array-11.patch1.53 KB

Alternative test, which checks the content of the returned array.

It uses $link += $book_manager->getLinkDefaults($link['nid']); to add the link defaults to the dummy data. This replicates some of what saveBookLink() does, but I guess that should be OK? If not, I could just hardcode what it adds in the test.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

+1

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Fixed

Committed 16fb258 and pushed to 8.x. Thanks!

  • Commit 16fb258 on 8.x by alexpott: 
    Issue #2208325 by mcjim, anksy: Incorrect error message when changing...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.