Support from Acquia helps fund testing for Drupal Acquia logo

Comments

choallin’s picture

Assigned: Unassigned » choallin

I would like to do this issue.

But I'm not 100% sure what you want to be done. Should I just write a test, and check if $node_links is set the right way?

ericmulder1980’s picture

Sorry to hijack this issue but i'd like to contribute to this issue. @choallin Perhaps we can team up in order to get this fixed.

I've looked into the linked issue and as far as i understand a change in the routing system breaks the implementation of hook_node_links_alter(). In this case it is the implementation of this hook in the book module. Other modules implementing this hook are comment, statistics and node.

As discussed with dawehner on #drupal-contribute the test should check if the node-links are added to the page WITH the proper querystring.

I will be working on this issue today.

ericmulder1980’s picture

Status: Active » Needs review
FileSize
1.14 KB

Status: Needs review » Needs work

The last submitted patch, 3: edit_issue_book_add-2353029-1.patch, failed testing.

choallin’s picture

Assigned: choallin » Unassigned
alexandre.todorov’s picture

Status: Needs work » Needs review
FileSize
2.62 KB

Added assertions to ensure links 'Add child page' and 'Printer-friendly version' are corrects.

Drupal Dev Days Mentor Sprint - Montpellier (France) 2015

googletorp’s picture

Cleaned up the code a bit to follow the Drupal code standards.

pingers’s picture

  1. +++ b/core/modules/book/src/Tests/BookTest.php
    @@ -146,6 +146,9 @@ function testBook() {
    +    // Check having an "Add child page" link
    

    Should be: Check that the "Add child page" exists.

  2. +++ b/core/modules/book/src/Tests/BookTest.php
    @@ -146,6 +146,9 @@ function testBook() {
    +    $this->checkBookNode($book, array($nodes[0], $nodes[3], $nodes[4]), FALSE, FALSE, $nodes[0], array());
    

    Short array syntax would be more readable. I.e. []

  3. +++ b/core/modules/book/src/Tests/BookTest.php
    @@ -246,6 +249,22 @@ function checkBookNode(EntityInterface $node, $nodes, $previous = FALSE, $up = F
    +      // Create the following URL: 'node/add/book?parent=' . $node->id();
    

    Maybe give an example in place of psuedo code.

  4. +++ b/core/modules/book/src/Tests/BookTest.php
    @@ -246,6 +249,22 @@ function checkBookNode(EntityInterface $node, $nodes, $previous = FALSE, $up = F
    +      $this->assertLinkByHref($url, 0, 'The add Child page link exists with the correct querystring');
    

    s/Child/child
    Also missing a period at the end of the assertion.

  5. +++ b/core/modules/book/src/Tests/BookTest.php
    @@ -246,6 +249,22 @@ function checkBookNode(EntityInterface $node, $nodes, $previous = FALSE, $up = F
    +      $this->assertLinkByHref($url, 0, 'The Printer-friendly version link exists with the correct path');
    

    Missing period at the end of the assertion.

priya.chat’s picture

Thanks @googletorp for the patch. I have made some changes to this with the changes mentioned in #8 .

@pingers, I have some query about your review comments in #8, Can you please explain your point 2 because its not clear to me. For rest of your points I have tried to solve them.

Please review the patch.

Thanks,

pingers’s picture

I just meant that [] is much shorter than array () and given how long that line is, short array syntax would be easier to read.

dawehner’s picture

It is really great to add some test coverage!!

+++ b/core/modules/book/src/Tests/BookTest.php
@@ -288,6 +291,24 @@ function checkBookNode(EntityInterface $node, $nodes, $previous = FALSE, $up = F
+      $url = \Drupal::url('node.add', array('node_type' => 'book'), array(
+        'query' => array('parent' => $node->id())
+      ));
...
+      $url = \Drupal::url('book.export', array('type' => 'html', 'node' => $node->id()));

I think we should use Url::fromRoute() instead here, see #2605546: Mark \Drupal::url() as deprecated

anil280988’s picture

Added Url::fromRoute() function and Short array syntax.

anil280988’s picture

FileSize
2.33 KB

Adding Interdiff file

Status: Needs review » Needs work

The last submitted patch, 12: drupal-add_child_page-2353029-12.patch, failed testing.

googletorp’s picture

Status: Needs work » Needs review
FileSize
444 bytes
3.83 KB

Added the missing use statement.

Status: Needs review » Needs work

The last submitted patch, 15: 2353029-15.patch, failed testing.

googletorp’s picture

Status: Needs work » Needs review
FileSize
3.83 KB
1.69 KB

Fixed a few syntax errors I missed from patch #12

Status: Needs review » Needs work

The last submitted patch, 17: 2353029-17.patch, failed testing.

snehi’s picture

@anil280988 i think correct syntax is
Url::fromRoute('entity.aggregator_feed.delete_form', ['aggregator_feed' => $feed->id()]),

Why extra

()

anil280988’s picture

Adding Short array syntax.

anil280988’s picture

Status: Needs work » Needs review

Changing status.
@snehi,
$node->id()
is the way to get the NID in D8.

The last submitted patch, 15: 2353029-15.patch, failed testing.

The last submitted patch, 12: drupal-add_child_page-2353029-12.patch, failed testing.

The last submitted patch, 17: 2353029-17.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 20: 2353029-20.patch, failed testing.

snehi’s picture

Status: Needs work » Needs review

Hi Anil,
Yes are 90 % right the actual code for printing node and its contents like nid, title and more.

$node = $this->routeMatch->getParameter('node');
    if ($node) {
    }

You can print anything in if statement. I hope that am making sense over here.

Thanks.

The last submitted patch, 15: 2353029-15.patch, failed testing.

The last submitted patch, 12: drupal-add_child_page-2353029-12.patch, failed testing.

The last submitted patch, 17: 2353029-17.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 20: 2353029-20.patch, failed testing.

aerozeppelin’s picture

Status: Needs work » Needs review
FileSize
3.86 KB
1.48 KB

Fix for failing tests in #20.

anil280988’s picture

Looks good to me. +1 for RTBC

Fred Martin’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Novice, -Needs tests

I have reviewed the patch, and it seems to be testing the proper things. I re-ran the tests on the d.o. infrastructure, and everything passed. When running the test locally, there was an unrelated failure in Drupal\book\Tests\BookTest->testBookDelete().

Fred Martin’s picture

I have reviewed the patch, and it seems to be testing the proper things. I re-ran the tests on the d.o. infrastructure, and everything passed. When running the test locally, there was an unrelated failure in Drupal\book\Tests\BookTest->testBookDelete().

catch’s picture

Status: Reviewed & tested by the community » Needs work

Looks good apart from a couple of small things:

  1. +++ b/core/modules/book/src/Tests/BookTest.php
    @@ -178,7 +179,7 @@ function testBook() {
    +    $this->checkBookNode($book, [$nodes[0], $nodes[3], $nodes[4]], FALSE, FALSE, $nodes[0], array());
    

    This change doesn't look necessary. Also it results in mixing [] and array() syntax since the last argument isn't updated.

  2. +++ b/core/modules/book/src/Tests/BookTest.php
    @@ -288,9 +292,27 @@ function checkBookNode(EntityInterface $node, $nodes, $previous = FALSE, $up = F
    +      // Suppose we have int value for $node->id() = 10;
    

    Is this comment necessary? Looks like it'll work regardless of the actual nid.

aerozeppelin’s picture

Status: Needs work » Needs review
FileSize
3.15 KB
1.37 KB

Changes as per #35

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

dawehner’s picture

Version: 8.1.x-dev » 8.3.x-dev
Status: Needs review » Reviewed & tested by the community

The feedback from @catch got addressed in #36, so back to RTBC

Given that this is a test it could be easily committed to 8.3.x and 8.2.x as well.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/book/src/Tests/BookTest.php
@@ -290,9 +294,26 @@ function checkBookNode(EntityInterface $node, $nodes, $previous = FALSE, $up = F
+    if ($this->loggedInUser->hasPermission('add content to books')) {
+      $this->assertLink('Add child page', 0, 'The add child page link appears on the page');
+      // Create the following URL: 'node/add/book?parent=10;
+      $url = Url::fromRoute('node.add', array('node_type' => 'book'), array(
+        'query' => array('parent' => $node->id())
+      ));
+
+      $this->assertLinkByHref($url->toString(), 0, 'The add child page link exists with the correct querystring.');
+    }
+
+    if ($this->loggedInUser->hasPermission('access printer-friendly version')) {
+      $this->assertLink('Printer-friendly version', 0, 'The Printer-friendly version link appears on the page.');
+      // Create the following URL: 'node/add/book?parent=' . $node->id();
+      $url = Url::fromRoute('book.export', array('type' => 'html', 'node' => $node->id()));
+      $this->assertLinkByHref($url->toString(), 0, 'The Printer-friendly version link exists with the correct path.');
+    }
+

We should be asserting the opposite case too. Ifs in test code generally mean we need this... so this should be something like:

    // Create the following URL: 'node/add/book?parent=10;
    $url = Url::fromRoute('node.add', array('node_type' => 'book'), array(
      'query' => array('parent' => $node->id())
    ));
    if ($this->loggedInUser->hasPermission('add content to books')) {
      $this->assertLink('Add child page', 0, 'The add child page link appears on the page');
      $this->assertLinkByHref($url->toString(), 0, 'The add child page link exists with the correct querystring.');
    }
    else {
      $this->assertNoLink('Add child page', 'The add child page link does not appear on the page');
      $this->assertNoLinkByHref($url->toString(), 'The add child page link with the correct querystring does not exist.');
    }

    // Create the following URL: 'node/add/book?parent=' . $node->id();
    $url = Url::fromRoute('book.export', array('type' => 'html', 'node' => $node->id()));
    if ($this->loggedInUser->hasPermission('access printer-friendly version')) {
      $this->assertLink('Printer-friendly version', 0, 'The Printer-friendly version link appears on the page.');
      $this->assertLinkByHref($url->toString(), 0, 'The Printer-friendly version link exists with the correct path.');
    }
    else {
      $this->assertNoLink('Printer-friendly version', 'The Printer-friendly version link does not appear on the page.');
      $this->assertLinkByHref($url->toString(), 'The Printer-friendly version link with the correct path does not exist.');
    }

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

quietone’s picture

Status: Needs work » Postponed

This extension is being deprecated, see #3376070: [Meta] Tasks to deprecate Book module. It will be removed from core and moved to a contrib project, #3376101: [11.x] [Meta] Tasks to remove Book.

This is now Postponed. The status is set according to two policies. The Remove a core extension and move it to a contributed project and the Extensions approved for removal policies.

This issue may be re-opened if it can be considered critical, If unsure, re-open the issue and ask in a comment.