Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
As pointed out in #2347465-83: Convert all instances of #type link/links to convert to use routes, I broke book_node_links_alter() but no tests failed.
Proposed resolution
We should add something to ensure the querystring is respected.
Remaining tasks
User interface changes
API changes
Comment | File | Size | Author |
---|---|---|---|
#36 | interdiff-2353029-31-36.txt | 1.37 KB | aerozeppelin |
#36 | 2353029-36.patch | 3.15 KB | aerozeppelin |
#31 | interdiff-2353029-20-31.txt | 1.48 KB | aerozeppelin |
#31 | 2353029-31.patch | 3.86 KB | aerozeppelin |
#20 | 2353029-20.patch | 3.82 KB | anil280988 |
Comments
Comment #1
choallin CreditAttribution: choallin commentedI 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?
Comment #2
ericmulder1980 CreditAttribution: ericmulder1980 commentedSorry 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.
Comment #3
ericmulder1980 CreditAttribution: ericmulder1980 commentedComment #5
choallin CreditAttribution: choallin commentedComment #6
alexandre.todorov CreditAttribution: alexandre.todorov commentedAdded assertions to ensure links 'Add child page' and 'Printer-friendly version' are corrects.
Drupal Dev Days Mentor Sprint - Montpellier (France) 2015
Comment #7
googletorp CreditAttribution: googletorp commentedCleaned up the code a bit to follow the Drupal code standards.
Comment #8
pingers CreditAttribution: pingers at University of Adelaide commentedShould be: Check that the "Add child page" exists.
Short array syntax would be more readable. I.e. []
Maybe give an example in place of psuedo code.
s/Child/child
Also missing a period at the end of the assertion.
Missing period at the end of the assertion.
Comment #9
priya.chat CreditAttribution: priya.chat at Publicis Sapient for Publicis Sapient commentedThanks @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,
Comment #10
pingers CreditAttribution: pingers at University of Adelaide commentedI just meant that [] is much shorter than array () and given how long that line is, short array syntax would be easier to read.
Comment #11
dawehnerIt is really great to add some test coverage!!
I think we should use
Url::fromRoute()
instead here, see #2605546: Mark \Drupal::url() as deprecatedComment #12
anil280988 CreditAttribution: anil280988 at Publicis Sapient for Publicis Sapient commentedAdded Url::fromRoute() function and Short array syntax.
Comment #13
anil280988 CreditAttribution: anil280988 at Publicis Sapient for Publicis Sapient commentedAdding Interdiff file
Comment #15
googletorp CreditAttribution: googletorp as a volunteer and commentedAdded the missing use statement.
Comment #17
googletorp CreditAttribution: googletorp as a volunteer and commentedFixed a few syntax errors I missed from patch #12
Comment #19
snehi CreditAttribution: snehi as a volunteer and at Publicis Sapient for Publicis Sapient commented@anil280988 i think correct syntax is
Url::fromRoute('entity.aggregator_feed.delete_form', ['aggregator_feed' => $feed->id()]),
Why extra
()
Comment #20
anil280988 CreditAttribution: anil280988 at Publicis Sapient for Publicis Sapient commentedAdding Short array syntax.
Comment #21
anil280988 CreditAttribution: anil280988 at Publicis Sapient for Publicis Sapient commentedChanging status.
@snehi,
$node->id()
is the way to get the NID in D8.
Comment #26
snehi CreditAttribution: snehi as a volunteer and at Publicis Sapient for Publicis Sapient commentedHi Anil,
Yes are 90 % right the actual code for printing node and its contents like nid, title and more.
You can print anything in if statement. I hope that am making sense over here.
Thanks.
Comment #31
aerozeppelin CreditAttribution: aerozeppelin commentedFix for failing tests in #20.
Comment #32
anil280988 CreditAttribution: anil280988 at Publicis Sapient for Publicis Sapient commentedLooks good to me. +1 for RTBC
Comment #33
Fred Martin CreditAttribution: Fred Martin as a volunteer commentedI 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()
.Comment #34
Fred Martin CreditAttribution: Fred Martin as a volunteer commentedI 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()
.Comment #35
catchLooks good apart from a couple of small things:
This change doesn't look necessary. Also it results in mixing [] and array() syntax since the last argument isn't updated.
Is this comment necessary? Looks like it'll work regardless of the actual nid.
Comment #36
aerozeppelin CreditAttribution: aerozeppelin commentedChanges as per #35
Comment #38
dawehnerThe 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.
Comment #39
alexpottWe should be asserting the opposite case too. Ifs in test code generally mean we need this... so this should be something like:
Comment #53
quietone CreditAttribution: quietone at PreviousNext commentedThis 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.