Convert book_export to a new-style Controller

Pulling to me

Finally a first version :)

Worked in my manual tests but I am unsure about the comments.

Patch before was faulty. Sorry. New version attached.

Third version with hopefully no mistakes...

Attached is a reroll

ignore the interdiff, I didn't change anything, so it's identical to the patch.

lost a use flag in the reroll. Adding back in.

+++ b/core/modules/book/book.moduleundefined
@@ -162,14 +162,6 @@ function book_menu() {
-    'access callback' => 'book_export_access',
-    'access arguments' => array(3),

This isn't addressed by the _permission: 'access printer-friendly version' added to the routing YAML, hence why node_access tests are failing by testbot. This will need an access checker class added to handle the node access stuff.

disasm, are you still working on this?

Going to work on this

+++ b/core/modules/book/book.moduleundefined
@@ -186,12 +186,8 @@ function book_menu() {
   $items['book/export/%/%node'] = array(
-    'page callback' => 'book_export',
-    'page arguments' => array(2, 3),
-    'access callback' => 'book_export_access',
-    'access arguments' => array(3),
+    'route_name' => 'book_export',
     'type' => MENU_CALLBACK,
-    'file' => 'book.pages.inc',

Menu callbacks now can be totally removed from hook_menu

+++ b/core/modules/book/lib/Drupal/book/Controller/BookController.phpundefined
@@ -86,4 +89,75 @@ public function bookRender() {
+   * @param \Drupal\node\Plugin\Core\Entity\EntityInterface $node
+   *   The node to export.
...
+  public function bookExport($type, EntityInterface $node) {
...
+  public function book_export_html(EntityInterface $node) {

Let's use NodeInterface in documentation/function signature.

+++ b/core/modules/book/lib/Drupal/book/Controller/BookController.phpundefined
@@ -86,4 +89,75 @@ public function bookRender() {
+    $type = drupal_strtolower($type);

Should be Unicode::strtolower($text)

+++ b/core/modules/book/lib/Drupal/book/Controller/BookController.phpundefined
@@ -86,4 +89,75 @@ public function bookRender() {
+
+    $export_function = 'book_export_' . $type;
+
+    if (function_exists($export_function)) {
+      print call_user_func($export_function, $node);
+    }

In order to call the export functions you have to be include book.pages.inc. (maybe you could convert them into a helper object). Additional: I think instead of printing there should be a response object returned.

+++ b/core/modules/book/lib/Drupal/book/Controller/BookController.phpundefined
@@ -86,4 +89,75 @@ public function bookRender() {
+   * @return
+   *   A string containing HTML representing the node and its children in
+   *   the book hierarchy.
...
+        return theme('book_export_html', array('title' => $node->label(), 'contents' => $contents, 'depth' => $node->book['depth']));

The proper way would be to return a render array.

+++ b/core/modules/book/lib/Drupal/book/Controller/BookController.phpundefined
@@ -86,4 +91,75 @@ public function bookRender() {
+      print call_user_func($export_function, $node);

It seems to be that it would be helpful to return a full reponse object?

+++ b/core/modules/book/lib/Drupal/book/Controller/BookController.phpundefined
@@ -86,4 +91,75 @@ public function bookRender() {
+  public function book_export_html(NodeInterface $node) {

lets make this method name exportHTML

+++ b/core/modules/book/lib/Drupal/book/Controller/BookController.phpundefined
@@ -86,4 +91,75 @@ public function bookRender() {
+        $tree = book_menu_subtree_data($node->book);
+        $contents = book_export_traverse($tree, 'book_node_export');

these should either be methods in this class or added to the book service.

While working on fixing stuff I realized something totally else. Book export should be using serializer instead of its own custom, hard to extend logic, but this would require certainly more effort.

This is just a patch which passes the tests.

#37: book-1938314-37.patch queued for re-testing.

This code looks very nice and clean! If we're not going to serializer as dawehner mentioned above, RTBC++. Adding Stalking Crell to get his thoughts on committing as is vs. switching to serializer.

I don't we should block this on converting to serializer now, as it might be complicated.

Actually... dawehner, couldn't this be replaced with an ANDed _permission and _entity_access on the route, no new checker needed? We spent all that time talking about it with chx in Portland, we may as well use that functionality. :-)

Yeah, once #1947880: Replace node_access() by $entity->access() is in, we can certainly do that.

So as the REST team is already full with work, getting a conversion going on is really unlikely, so we might just live with what we have at the moment, which means that it should be set to needs work again?

OK, then maybe that's a follow up and we can just drop a @todo on it.

This is regarding using && and _entity_access

I propose to move serializing responsibility out of controller and register that class in container. In this way if someone would like to extend that functionality they can swap class in container.

Patch corrected according to #48.

+++ b/core/modules/book/lib/Drupal/book/Access/BookAccessCheck.phpundefined
@@ -0,0 +1,34 @@
+    return user_access('access printer-friendly version') && node_access('view', $request->attributes->get('node'));

This should be using the user on the request and the hasPermission() method on the account interface.

+++ b/core/modules/book/lib/Drupal/book/BookExport.phpundefined
@@ -0,0 +1,95 @@
+    if (!user_access('access printer-friendly version')) {
+      throw new AccessDeniedHttpException();
+    }

As above... also do we really need to do this here considering the BookAccessCheck already does this?

+++ b/core/modules/book/lib/Drupal/book/BookExport.phpundefined
@@ -0,0 +1,95 @@
+    $contents = $this->exportTraverse($tree, 'book_node_export');

Let's refactor book_node_export() into the BookExport class as well as it is only called from methods already refactored to this class.

+++ b/core/modules/book/lib/Drupal/book/BookExport.phpundefined
@@ -0,0 +1,95 @@
+    return theme('book_export_html', array('title' => $node->label(), 'contents' => $contents, 'depth' => $node->book['depth']));

Can we not return a render array here?

+++ b/core/modules/book/lib/Drupal/book/BookExport.phpundefined
@@ -0,0 +1,95 @@
+   * @param string $visit_func
+   *   A function callback to be called upon visiting a node in the tree.

This should be @param callable $visit_func as we can pass an array instead of a string to call a method on the class.

This should cover @alexpott's review.

+++ b/core/modules/book/lib/Drupal/book/BookExport.phpundefined
@@ -0,0 +1,138 @@
+    $tree = book_menu_subtree_data($node->book);

Can we move this function to bookManger?

+++ b/core/modules/book/lib/Drupal/book/Controller/BookController.phpundefined
@@ -86,4 +99,40 @@ public function bookRender() {
+   * The function delegates the generation of output to helper functions. The
+   * function name is derived by prepending 'book_export_' to the given output
+   * type. So, e.g., a type of 'html' results in a call to the function
+   * book_export_html().

This doc block needs update.

book_menu_subtree_data() is used in a bunch of places, I think that could be its own conversion.
Or maybe after I get this green.

node_access() has logic that $node->access() doesn't, that's messed up.

Don't see any need to bother Crell with this issue.

Will re-test.

Patch still applies.
Code looks good.
When I manually tested the patch, though, it doesn't act the same as it did before the patch.

My manual test steps are:

1. Install D8.
2. Enable the book module.
3. Create ~10 or so book nodes.
4. Put the book nodes in some sort of book structure.
5. Visit any page in the book.
6. In the bottom-right corner, under the book page navigation, click "Printer-friendly version".

Before applying the patch, clicking the "Printer-friendly version" link in the bottom-right of the page would display the book with no theme or Drupal page wrapper at all.

After applying the patch, clicking the "Printer-friendly version" link in the bottom-right of the page displays the book in the content area of a normal Drupal page (i.e.: themed and with the Drupal page wrapper). While that functionality may be desirable to some people, I think that decision and the work around it is out-of-scope for this issue.

Unfortunately, that means this issue gets bumped back to "needs work".

Leaving this assigned to myself so I can fix it. :)

I talked briefly with @Crell about how to fix this.

He was thinking that we should change the route defaults from _content to _controller, and adjust the controller object to return a response object containing the HTML.

Try this...

*sad trombone* will fix.

Try this...

Or rather, this...

So, it turns out that interdiffs are not good substitutes for patches. Thanks @disasm! Let's try this again...

Interdiff is between this and #61.

We can't inject in entity classes yet.

+++ b/core/modules/book/lib/Drupal/book/BookExport.php
@@ -0,0 +1,140 @@
+    if (!isset($node->book)) {
+      throw new NotFoundHttpException();
+    }

I am wondering whether a notfoundhttpexception should be thrown in such a enviroment and not in the actual controller ...

This patch needed a reroll and per talking to dawehner in IRC, I changed the exception class to just plain Exception.

#70: book-new_book_export_controller-1938314-70.patch queued for re-testing.

@wamilton
Just in general it would be nice to provide an interdiff all the time.

reroll of #64. Interdiff is changes from #70 (changing NotFoundException to Exception).

Mh, I actually thought that we still throw NotFoundException on the controller level.

I could definitely be wrong here, but after reading the patch in #75, I get the impression it's throwing the correct exceptions. The controller uses HttpNotFound while the export code that shouldn't know anything about httpuses a plain exception.

That looks right to me. :)

Oh, well, maybe it was just too late/early.

No longer applies.

Straight reroll. Nothing actually broke, just patch context

Needs reroll

git ac https://drupal.org/files/book-1938314-81.patch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 20174  100 20174    0     0  16348      0  0:00:01  0:00:01 --:--:-- 19140
error: patch failed: core/modules/book/lib/Drupal/book/Controller/BookController.php:7
error: core/modules/book/lib/Drupal/book/Controller/BookController.php: patch does not apply

Just use statement conflict nothing big.

PS: I am going to reroll some patches while @tim.plunkett is sleeping :D

Patch no longer applies.

Straight re-roll.

+++ b/core/modules/book/book.services.yml
@@ -2,3 +2,6 @@ services:
+    arguments: ['@plugin.manager.entity']

this should be @entity.manager now.

Moving back to RTBC after reroll. There are a number of improvements to clean this up that could be done, but we're trying to get these committed as quickly as possible so we can get rid of the old menu router. I would like to see a follow-up issue to extend ControllerBase, use $this->t(), and use proper injection instead of calling \Drupal::moduleHander() in classes, but after we get all of the book module using the new router, we could do a single issue for cleanup of that module.

Agreed; we need to get these things converted sooner than later, so let's leave polishing niceties for follow-up issues.

Committed and pushed to 8.x. Thanks!