Convert the book_export page callback to a new-style Controller, using the instructions on http://drupal.org/node/1800686

CommentFileSizeAuthor
#87 drupal8.book-module.1938314-87.patch19.68 KBjibran
#85 drupal8.book-module.1938314-85.patch19.69 KBLetharion
#83 drupal8.book-module.1938314-83.patch19.7 KBjibran
#81 book-1938314-81.patch19.7 KBtim.plunkett
#75 drupal8.new_book_export_controller.1938314-74.patch20.62 KBdisasm
#75 interdiff.txt1.03 KBdisasm
#70 book-new_book_export_controller-1938314-70.patch21.27 KBwamilton
#64 book-new_book_export_controller-1938314-64.patch20.75 KBmparker17
#64 interdiff.txt683 bytesmparker17
#62 book-new_book_export_controller-1938314-62.patch683 bytesmparker17
#62 interdiff.txt683 bytesmparker17
#61 book-new_book_export_controller-1938314-61.patch20.75 KBmparker17
#61 interdiff.txt645 bytesmparker17
#58 book-new_book_export_controller-1938314-57.patch20.71 KBmparker17
#58 interdiff.txt1.33 KBmparker17
#54 book-1938314-54.patch20.63 KBtim.plunkett
#54 interdiff.txt11.48 KBtim.plunkett
#52 book-1938314-52.patch14.77 KBtim.plunkett
#52 interdiff.txt8.87 KBtim.plunkett
#49 book-1938314-49.patch13.94 KBygerasimov
#49 book-1938314-49-interdiff.txt1.54 KBygerasimov
#46 book-1938314-46.patch13.13 KBygerasimov
#46 book-export-1938314-46-interdiff.txt9.61 KBygerasimov
#37 book-1938314-37.patch12.53 KBdawehner
#32 book-export-1938314-32.patch9.47 KBkgoel
#32 interdiff.txt1.65 KBkgoel
#29 book-export-1938314-28.patch7.82 KBkgoel
#26 book-export-1938314-26.patch8.8 KBAnonymous (not verified)
#23 1938314-23.patch7.73 KBAnonymous (not verified)
#23 interdiff-20-23.txt1.28 KBAnonymous (not verified)
#20 book-export-1938314-20.patch7.65 KBkgoel
#20 interdiff.txt667 byteskgoel
#18 book-export-1938314-18.patch7.66 KBkgoel
#13 drupal-book_export-1938314-13.patch4.73 KBdisasm
#13 interdiff.txt527 bytesdisasm
#10 drupal-book_export-1938314-10.patch4.68 KBdisasm
#10 interdiff.txt4.68 KBdisasm
#6 1938314-Convert_book_export.patch6.72 KBKars-T
#5 1938314-Convert_book_export.patch3.82 KBKars-T
#4 1938314-Convert_book_export.patch6.87 KBKars-T
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

DamienMcKenna’s picture

Issue tags: +SprintWeekend2013
Kars-T’s picture

Assigned: Unassigned » Kars-T
Issue tags: -SprintWeekend2013

Pulling to me

DamienMcKenna’s picture

Issue tags: +SprintWeekend2013
Kars-T’s picture

Status: Active » Needs review
FileSize
6.87 KB

Finally a first version :)

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

Kars-T’s picture

Patch before was faulty. Sorry. New version attached.

Kars-T’s picture

Third version with hopefully no mistakes...

Crell’s picture

We discussed in IRC that we need to change the callback mechanism for the type-specific importers, but that can happen separately. #6 looks good to me visually. I'll wait for testbot before RTBCing it.

Crell’s picture

Status: Needs review » Reviewed & tested by the community

The bots are overloaded today, and webchick said to go ahead, so...

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 1938314-Convert_book_export.patch, failed testing.

disasm’s picture

Status: Needs work » Needs review
FileSize
4.68 KB
4.68 KB

Attached is a reroll

disasm’s picture

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

Status: Needs review » Needs work

The last submitted patch, drupal-book_export-1938314-10.patch, failed testing.

disasm’s picture

Status: Needs work » Needs review
FileSize
527 bytes
4.73 KB

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

disasm’s picture

+++ 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.

Status: Needs review » Needs work

The last submitted patch, drupal-book_export-1938314-13.patch, failed testing.

kim.pepper’s picture

disasm, are you still working on this?

kgoel’s picture

Assigned: Kars-T » kgoel

Going to work on this

kgoel’s picture

Status: Needs work » Needs review
FileSize
7.66 KB

Status: Needs review » Needs work

The last submitted patch, book-export-1938314-18.patch, failed testing.

kgoel’s picture

Status: Needs work » Needs review
FileSize
667 bytes
7.65 KB

Status: Needs review » Needs work

The last submitted patch, book-export-1938314-20.patch, failed testing.

dawehner’s picture

+++ 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.

Anonymous’s picture

Status: Needs work » Needs review
FileSize
1.28 KB
7.73 KB

Reroll the #20 patch as per the suggestions in the #22

  • Menu callbacks now can be totally removed from hook_menu
  • Unicode::strtolower($text)

Can anybody elaborate on these so that I can also work on these suggestions.

  • Let's use NodeInterface in documentation/function signature
  • Let's use NodeInterface in documentation/function signature

Need Help in these.Your help will be appreciated.

Crell’s picture

+++ b/core/modules/book/book.module
@@ -186,12 +186,7 @@ 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),
-    'type' => MENU_CALLBACK,
-    'file' => 'book.pages.inc',
+    'route_name' => 'book_export',
   );

This entire menu item can be removed completely.

+++ b/core/modules/book/lib/Drupal/book/Controller/BookController.php
@@ -86,4 +90,75 @@ public function bookRender() {
+  public function bookExport($type, EntityInterface $node) {

Instead of EntityInterface, the $node variable should be type hinted with NodeInterface. The "use" statements above will then also need to be updated to include Drupal\node\NodeInterface.

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

Same as above.

$type = drupal_strtolower($type); can be replaced with $type = Unicode::strtolower($type); Again, you'll need a "use" statement for it.

Status: Needs review » Needs work

The last submitted patch, 1938314-23.patch, failed testing.

Anonymous’s picture

Status: Needs work » Needs review
FileSize
8.8 KB

Applied all the suggestions given in #24.Please Needs Review.

Status: Needs review » Needs work
Issue tags: -SprintWeekend2013, -WSCCI-conversion

The last submitted patch, book-export-1938314-26.patch, failed testing.

Anonymous’s picture

Status: Needs work » Needs review
Issue tags: +SprintWeekend2013, +WSCCI-conversion

#26: book-export-1938314-26.patch queued for re-testing.

kgoel’s picture

Status: Needs review » Needs work

The last submitted patch, book-export-1938314-28.patch, failed testing.

Crell’s picture

+++ b/core/modules/book/book.routing.yml
@@ -18,3 +18,10 @@ book_settings:
+book_export:
+  pattern: '/book/export/{type}/{node}'
+  defaults:
+    _content: '\Drupal\book\Controller\BookController::bookExport'
+  requirements:
+    _permission: 'access printer-friendly version'

The route needs a custom access checker to match the logic in the old menu item. That's why the access test is failing. See https://drupal.org/node/1851490 for instructions on how to do that.

I don't know what the title errors are all about...

kgoel’s picture

Status: Needs work » Needs review
FileSize
1.65 KB
9.47 KB

Status: Needs review » Needs work

The last submitted patch, book-export-1938314-32.patch, failed testing.

dawehner’s picture

+++ 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?

Crell’s picture

Not just helpful; required. :-) Crell-- for not catching that before.

+++ b/core/modules/book/book.routing.yml
@@ -18,3 +18,10 @@ book_settings:
+book_export:
+  pattern: '/book/export/{type}/{node}'
+  defaults:
+    _content: '\Drupal\book\Controller\BookController::bookExport'
+  requirements:
+    _permission: 'access printer-friendly version'

Nothing here is connecting the new access checker. What you need to do instead of specifying a permission is specify a requirement of _book_export_access: 'TRUE', which you can then check for in the applies() method (note the _ prefix).

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

As a method this isn't going to work. the bookExport() method still expects it to be a function. Sucks, but we can't fix that here.

disasm’s picture

+++ 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.

dawehner’s picture

Status: Needs work » Needs review
FileSize
12.53 KB

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.

Status: Needs review » Needs work
Issue tags: -SprintWeekend2013, -WSCCI-conversion

The last submitted patch, book-1938314-37.patch, failed testing.

stella’s picture

Status: Needs work » Needs review
Issue tags: +SprintWeekend2013, +WSCCI-conversion

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

disasm’s picture

Issue tags: +Stalking Crell

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.

dawehner’s picture

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

Crell’s picture

Status: Needs review » Needs work
+++ b/core/modules/book/lib/Drupal/book/Access/BookExport.php
@@ -0,0 +1,34 @@
+  public function access(Route $route, Request $request) {
+    return user_access('access printer-friendly version') && node_access('view', $request->attributes->get('node'));

Actually... dawhner, 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. :-)

+++ b/core/modules/book/lib/Drupal/book/Controller/BookController.php
@@ -86,4 +92,118 @@ public function bookRender() {
+    // @todo Conver the custom export functionality to serializer.
+    $method = 'bookExport' . $type;
+    if (method_exists($this, $method)) {
+      return new Response($this->{$method}($node));
+    }
+    else {
+      drupal_set_message(t('Unknown export format.'));
+      throw new NotFoundHttpException();

Isn't this a regression? If we insist that the format be a method on this class, contrib can no longer add new formatters here. Mind you, I don't know that contrib ever did so, and I don't know why it would ever want to, but...

I definitely agree on punting on serializer. I don't know what would be involved there, but probably more than we want to deal with. Plus, I'd figured this would end up as plugins. That said... since this is such an obscure corner of the code it would not surprise me if we could change it later (serializer or plugins or both) without it qualifying as an API change. And if not, meh, it's not the end of the world.

dawehner’s picture

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?

dawehner’s picture

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

This is regarding using && and _entity_access

Crell’s picture

Bah. I see. OK, we should probably have a node access-specific checker, but we can punt that I guess. What about the formatter regression?

ygerasimov’s picture

Assigned: kgoel » Unassigned
Status: Needs work » Needs review
FileSize
9.61 KB
13.13 KB

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.

Crell’s picture

Status: Needs review » Reviewed & tested by the community

meh. I can live with that.

Crell’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/book/lib/Drupal/book/Controller/BookController.php
@@ -86,4 +90,41 @@ public function bookRender() {
+    $book_export_service = \Drupal::service('book.export');

Wait, no, I can't live with that. This should be properly injected.

ygerasimov’s picture

Status: Needs work » Needs review
FileSize
1.54 KB
13.94 KB

Patch corrected according to #48.

Crell’s picture

Status: Needs review » Reviewed & tested by the community

Now I can live with that. :-)

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ 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.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
8.87 KB
14.77 KB

This should cover @alexpott's review.

jibran’s picture

Status: Needs review » Needs work
+++ 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.

tim.plunkett’s picture

Status: Needs work » Needs review
Issue tags: -Stalking Crell
FileSize
11.48 KB
20.63 KB

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.

mparker17’s picture

Assigned: Unassigned » mparker17

Will re-test.

mparker17’s picture

Status: Needs review » Needs work

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".

mparker17’s picture

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.

mparker17’s picture

Assigned: mparker17 » Unassigned
Status: Needs work » Needs review
FileSize
1.33 KB
20.71 KB

Try this...

Status: Needs review » Needs work

The last submitted patch, book-new_book_export_controller-1938314-57.patch, failed testing.

mparker17’s picture

Assigned: Unassigned » mparker17

*sad trombone* will fix.

mparker17’s picture

Assigned: mparker17 » Unassigned
Status: Needs work » Needs review
FileSize
645 bytes
20.75 KB

Try this...

mparker17’s picture

Or rather, this...

Status: Needs review » Needs work

The last submitted patch, book-new_book_export_controller-1938314-62.patch, failed testing.

mparker17’s picture

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.

mparker17’s picture

Status: Needs work » Needs review
Crell’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/node/lib/Drupal/node/Plugin/Core/Entity/Node.php
    @@ -145,6 +147,39 @@ public static function preDelete(EntityStorageControllerInterface $storage_contr
    +    return \Drupal::entityManager()
    +      ->getAccessController($this->entityType)
    +      ->access($this, $operation, $this->prepareLangcode(), $account);
    

    Ak! How'd this get through? \Drupal is not allowed inside classes. It should be injected.

  2. +++ b/core/modules/node/lib/Drupal/node/Plugin/Core/Entity/Node.php
    @@ -145,6 +147,39 @@ public static function preDelete(EntityStorageControllerInterface $storage_contr
    +    if (\Drupal::moduleHandler()->moduleExists('language')) {
    +      // Load languages the node exists in.
    +      $node_translations = $this->getTranslationLanguages();
    +      // Load the language from content negotiation.
    +      $content_negotiation_langcode = \Drupal::languageManager()->getLanguage(Language::TYPE_CONTENT)->id;
    +      // If there is a translation available, use it.
    +      if (isset($node_translations[$content_negotiation_langcode])) {
    +        $langcode = $content_negotiation_langcode;
    +      }
    +    }
    

    Ibid.

tim.plunkett’s picture

We can't inject in entity classes yet.

dawehner’s picture

+++ 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 ...

Crell’s picture

tim: Ugh. Still? Fine.

dawehner: You are correct, an *HttpException shouldn't be thrown below the controller level.

wamilton’s picture

Status: Needs work » Needs review
FileSize
21.27 KB

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

Status: Needs review » Needs work
Issue tags: -SprintWeekend2013, -WSCCI-conversion

The last submitted patch, book-new_book_export_controller-1938314-70.patch, failed testing.

disasm’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: +SprintWeekend2013, +WSCCI-conversion

The last submitted patch, book-new_book_export_controller-1938314-70.patch, failed testing.

dawehner’s picture

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

disasm’s picture

Status: Needs work » Needs review
FileSize
1.03 KB
20.62 KB

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

dawehner’s picture

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

Letharion’s picture

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. :)

dawehner’s picture

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

Crell’s picture

Status: Needs review » Reviewed & tested by the community

This is going to conflict with #2057589: [Docs clean-up] Rename ControllerInterface to ContainerInjectionInterface, but at least for posterity...

webchick’s picture

Status: Reviewed & tested by the community » Needs work

No longer applies.

tim.plunkett’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
19.7 KB

Straight reroll. Nothing actually broke, just patch context

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

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
jibran’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
19.7 KB

Just use statement conflict nothing big.

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

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Patch no longer applies.

Letharion’s picture

Status: Needs work » Needs review
FileSize
19.69 KB

Straight re-roll.

jibran’s picture

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

this should be @entity.manager now.

jibran’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
19.68 KB
disasm’s picture

Status: Needs review » Reviewed & tested by the community

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.

Crell’s picture

disasm: I think #2077513: Refactor ControllerBase to be more consistent with FormBase is what you're looking for. (Not a blocker here.)

webchick’s picture

Status: Reviewed & tested by the community » Fixed

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!

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