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

Files: 
CommentFileSizeAuthor
#87 drupal8.book-module.1938314-87.patch19.68 KBjibran
PASSED: [[SimpleTest]]: [MySQL] 58,083 pass(es).
[ View ]
#85 drupal8.book-module.1938314-85.patch19.69 KBLetharion
PASSED: [[SimpleTest]]: [MySQL] 58,066 pass(es).
[ View ]
#83 drupal8.book-module.1938314-83.patch19.7 KBjibran
PASSED: [[SimpleTest]]: [MySQL] 58,385 pass(es).
[ View ]
#81 book-1938314-81.patch19.7 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,447 pass(es).
[ View ]
#75 drupal8.new_book_export_controller.1938314-74.patch20.62 KBdisasm
PASSED: [[SimpleTest]]: [MySQL] 58,136 pass(es).
[ View ]
#75 interdiff.txt1.03 KBdisasm
#70 book-new_book_export_controller-1938314-70.patch21.27 KBwamilton
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed during invocation of run-tests.sh.
[ View ]
#64 book-new_book_export_controller-1938314-64.patch20.75 KBmparker17
PASSED: [[SimpleTest]]: [MySQL] 58,084 pass(es).
[ View ]
#64 interdiff.txt683 bytesmparker17
#62 book-new_book_export_controller-1938314-62.patch683 bytesmparker17
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch book-new_book_export_controller-1938314-62.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#62 interdiff.txt683 bytesmparker17
#61 book-new_book_export_controller-1938314-61.patch20.75 KBmparker17
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#61 interdiff.txt645 bytesmparker17
#58 book-new_book_export_controller-1938314-57.patch20.71 KBmparker17
FAILED: [[SimpleTest]]: [MySQL] 58,275 pass(es), 0 fail(s), and 9 exception(s).
[ View ]
#58 interdiff.txt1.33 KBmparker17
#54 book-1938314-54.patch20.63 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 57,817 pass(es).
[ View ]
#54 interdiff.txt11.48 KBtim.plunkett
#52 book-1938314-52.patch14.77 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 57,504 pass(es), 27 fail(s), and 0 exception(s).
[ View ]
#52 interdiff.txt8.87 KBtim.plunkett
#49 book-1938314-49.patch13.94 KBygerasimov
PASSED: [[SimpleTest]]: [MySQL] 57,433 pass(es).
[ View ]
#49 book-1938314-49-interdiff.txt1.54 KBygerasimov
#46 book-1938314-46.patch13.13 KBygerasimov
PASSED: [[SimpleTest]]: [MySQL] 57,196 pass(es).
[ View ]
#46 book-export-1938314-46-interdiff.txt9.61 KBygerasimov
#37 book-1938314-37.patch12.53 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 58,133 pass(es).
[ View ]
#32 book-export-1938314-32.patch9.47 KBkgoel
FAILED: [[SimpleTest]]: [MySQL] 58,039 pass(es), 27 fail(s), and 0 exception(s).
[ View ]
#32 interdiff.txt1.65 KBkgoel
#29 book-export-1938314-28.patch7.82 KBkgoel
FAILED: [[SimpleTest]]: [MySQL] 58,065 pass(es), 27 fail(s), and 0 exception(s).
[ View ]
#26 book-export-1938314-26.patch8.8 KBnick_daffodil
FAILED: [[SimpleTest]]: [MySQL] 57,682 pass(es), 27 fail(s), and 0 exception(s).
[ View ]
#23 1938314-23.patch7.73 KBnick_daffodil
FAILED: [[SimpleTest]]: [MySQL] 58,098 pass(es), 27 fail(s), and 11 exception(s).
[ View ]
#23 interdiff-20-23.txt1.28 KBnick_daffodil
#20 book-export-1938314-20.patch7.65 KBkgoel
FAILED: [[SimpleTest]]: [MySQL] 55,998 pass(es), 27 fail(s), and 11 exception(s).
[ View ]
#20 interdiff.txt667 byteskgoel
#18 book-export-1938314-18.patch7.66 KBkgoel
FAILED: [[SimpleTest]]: [MySQL] 55,396 pass(es), 28 fail(s), and 0 exception(s).
[ View ]
#13 drupal-book_export-1938314-13.patch4.73 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] 54,485 pass(es), 6 fail(s), and 23 exception(s).
[ View ]
#13 interdiff.txt527 bytesdisasm
#10 drupal-book_export-1938314-10.patch4.68 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] 54,512 pass(es), 6 fail(s), and 23 exception(s).
[ View ]
#10 interdiff.txt4.68 KBdisasm
#6 1938314-Convert_book_export.patch6.72 KBKars-T
FAILED: [[SimpleTest]]: [MySQL] 52,796 pass(es), 2 fail(s), and 11 exception(s).
[ View ]
#5 1938314-Convert_book_export.patch3.82 KBKars-T
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#4 1938314-Convert_book_export.patch6.87 KBKars-T
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1938314-Convert_book_export.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Comments

Issue tags:+#SprintWeekend

Assigned:Unassigned» Kars-T
Issue tags:-#SprintWeekend

Pulling to me

Issue tags:+#SprintWeekend

Status:Active» Needs review
StatusFileSize
new6.87 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1938314-Convert_book_export.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Finally a first version :)

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

StatusFileSize
new3.82 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Patch before was faulty. Sorry. New version attached.

StatusFileSize
new6.72 KB
FAILED: [[SimpleTest]]: [MySQL] 52,796 pass(es), 2 fail(s), and 11 exception(s).
[ View ]

Third version with hopefully no mistakes...

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.

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.

Status:Needs work» Needs review
StatusFileSize
new4.68 KB
new4.68 KB
FAILED: [[SimpleTest]]: [MySQL] 54,512 pass(es), 6 fail(s), and 23 exception(s).
[ View ]

Attached is a reroll

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.

Status:Needs work» Needs review
StatusFileSize
new527 bytes
new4.73 KB
FAILED: [[SimpleTest]]: [MySQL] 54,485 pass(es), 6 fail(s), and 23 exception(s).
[ View ]

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.

Status:Needs review» Needs work

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

disasm, are you still working on this?

Assigned:Kars-T» kgoel

Going to work on this

Status:Needs work» Needs review
StatusFileSize
new7.66 KB
FAILED: [[SimpleTest]]: [MySQL] 55,396 pass(es), 28 fail(s), and 0 exception(s).
[ View ]

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new667 bytes
new7.65 KB
FAILED: [[SimpleTest]]: [MySQL] 55,998 pass(es), 27 fail(s), and 11 exception(s).
[ View ]

Status:Needs review» Needs work

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

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

Status:Needs work» Needs review
StatusFileSize
new1.28 KB
new7.73 KB
FAILED: [[SimpleTest]]: [MySQL] 58,098 pass(es), 27 fail(s), and 11 exception(s).
[ View ]

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.

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

Status:Needs work» Needs review
StatusFileSize
new8.8 KB
FAILED: [[SimpleTest]]: [MySQL] 57,682 pass(es), 27 fail(s), and 0 exception(s).
[ View ]

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

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

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

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

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

StatusFileSize
new7.82 KB
FAILED: [[SimpleTest]]: [MySQL] 58,065 pass(es), 27 fail(s), and 0 exception(s).
[ View ]

Status:Needs review» Needs work

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

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

Status:Needs work» Needs review
StatusFileSize
new1.65 KB
new9.47 KB
FAILED: [[SimpleTest]]: [MySQL] 58,039 pass(es), 27 fail(s), and 0 exception(s).
[ View ]

Status:Needs review» Needs work

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

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

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.

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

Status:Needs work» Needs review
StatusFileSize
new12.53 KB
PASSED: [[SimpleTest]]: [MySQL] 58,133 pass(es).
[ View ]

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:-#SprintWeekend, -WSCCI-conversion

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

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

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

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.

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

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.

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

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?

Assigned:kgoel» Unassigned
Status:Needs work» Needs review
StatusFileSize
new9.61 KB
new13.13 KB
PASSED: [[SimpleTest]]: [MySQL] 57,196 pass(es).
[ View ]

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.

Status:Needs review» Reviewed & tested by the community

meh. I can live with that.

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.

Status:Needs work» Needs review
StatusFileSize
new1.54 KB
new13.94 KB
PASSED: [[SimpleTest]]: [MySQL] 57,433 pass(es).
[ View ]

Patch corrected according to #48.

Status:Needs review» Reviewed & tested by the community

Now I can live with that. :-)

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.

Status:Needs work» Needs review
StatusFileSize
new8.87 KB
new14.77 KB
FAILED: [[SimpleTest]]: [MySQL] 57,504 pass(es), 27 fail(s), and 0 exception(s).
[ View ]

This should cover @alexpott's review.

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.

Status:Needs work» Needs review
Issue tags:-Stalking Crell
StatusFileSize
new11.48 KB
new20.63 KB
PASSED: [[SimpleTest]]: [MySQL] 57,817 pass(es).
[ View ]

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.

Assigned:Unassigned» mparker17

Will re-test.

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

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.

Assigned:mparker17» Unassigned
Status:Needs work» Needs review
StatusFileSize
new1.33 KB
new20.71 KB
FAILED: [[SimpleTest]]: [MySQL] 58,275 pass(es), 0 fail(s), and 9 exception(s).
[ View ]

Try this...

Status:Needs review» Needs work

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

Assigned:Unassigned» mparker17

*sad trombone* will fix.

Assigned:mparker17» Unassigned
Status:Needs work» Needs review
StatusFileSize
new645 bytes
new20.75 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Try this...

StatusFileSize
new683 bytes
new683 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch book-new_book_export_controller-1938314-62.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Or rather, this...

Status:Needs review» Needs work

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

StatusFileSize
new683 bytes
new20.75 KB
PASSED: [[SimpleTest]]: [MySQL] 58,084 pass(es).
[ View ]

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.

Status:Needs work» Needs review

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.

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

tim: Ugh. Still? Fine.

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

Status:Needs work» Needs review
StatusFileSize
new21.27 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed during invocation of run-tests.sh.
[ View ]

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:-#SprintWeekend, -WSCCI-conversion

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

Status:Needs work» Needs review

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

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

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

Status:Needs work» Needs review
StatusFileSize
new1.03 KB
new20.62 KB
PASSED: [[SimpleTest]]: [MySQL] 58,136 pass(es).
[ View ]

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.

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

Status:Reviewed & tested by the community» Needs work

No longer applies.

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new19.7 KB
PASSED: [[SimpleTest]]: [MySQL] 58,447 pass(es).
[ View ]

Straight reroll. Nothing actually broke, just patch context

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

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new19.7 KB
PASSED: [[SimpleTest]]: [MySQL] 58,385 pass(es).
[ View ]

Just use statement conflict nothing big.

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

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

Patch no longer applies.

Status:Needs work» Needs review
StatusFileSize
new19.69 KB
PASSED: [[SimpleTest]]: [MySQL] 58,066 pass(es).
[ View ]

Straight re-roll.

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.

Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new19.68 KB
PASSED: [[SimpleTest]]: [MySQL] 58,083 pass(es).
[ View ]

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.

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

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.