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

Files: 
CommentFileSizeAuthor
#59 book-1945390-59.patch16.87 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 58,793 pass(es).
[ View ]
#54 interdiff.txt7.11 KBtim.plunkett
#54 book-1945390-54.patch16.69 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch book-1945390-54.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#50 drupal8.book-module.1945390-50.patch18.48 KBdisasm
PASSED: [[SimpleTest]]: [MySQL] 58,437 pass(es).
[ View ]
#50 interdiff.txt2.18 KBdisasm
#48 drupal8.book-module.1945390-48.patch18.29 KBdisasm
PASSED: [[SimpleTest]]: [MySQL] 58,438 pass(es).
[ View ]
#48 interdiff.txt1.84 KBdisasm
#46 drupal8.book-module.1945390-46.patch18.23 KBdisasm
PASSED: [[SimpleTest]]: [MySQL] 58,353 pass(es).
[ View ]
#46 interdiff.txt3.46 KBdisasm
#43 drupal8.book-module.1945390-43.patch17.6 KBdisasm
PASSED: [[SimpleTest]]: [MySQL] 58,397 pass(es).
[ View ]
#43 interdiff.txt5.42 KBdisasm
#40 interdiff.txt1.47 KBAlan Evans
#40 drupal-book-admin-edit-1945390-40.patch17.16 KBAlan Evans
PASSED: [[SimpleTest]]: [MySQL] 57,419 pass(es).
[ View ]
#38 drupal-book-admin-edit-1945390-38.patch17.57 KBAlan Evans
PASSED: [[SimpleTest]]: [MySQL] 56,964 pass(es).
[ View ]
#31 interdiff.txt2.02 KBAlan Evans
#31 drupal-book-admin-edit-1945390-31.patch17.38 KBAlan Evans
PASSED: [[SimpleTest]]: [MySQL] 55,820 pass(es).
[ View ]
#28 interdiff.txt1.84 KBAlan Evans
#28 drupal-book-admin-edit-1945390-28.patch17.59 KBAlan Evans
PASSED: [[SimpleTest]]: [MySQL] 55,263 pass(es).
[ View ]
#20 interdiff.txt1.13 KBAlan Evans
#20 drupal-book-admin-edit-1945390-20.patch17.44 KBAlan Evans
PASSED: [[SimpleTest]]: [MySQL] 54,151 pass(es).
[ View ]
#19 interdiff.txt1.15 KBAlan Evans
#19 drupal-book-admin-edit-1945390-19.patch17.47 KBAlan Evans
PASSED: [[SimpleTest]]: [MySQL] 54,192 pass(es).
[ View ]
#16 drupal-book-admin-edit-1945390-16.patch17.91 KBAlan Evans
PASSED: [[SimpleTest]]: [MySQL] 53,995 pass(es).
[ View ]
#16 interdiff.txt1.01 KBAlan Evans
#13 interdiff.txt1.09 KBAlan Evans
#13 drupal-book-admin-edit-1945390-13.patch17.85 KBAlan Evans
PASSED: [[SimpleTest]]: [MySQL] 53,854 pass(es).
[ View ]
#7 drupal-book-admin-edit-1945390-7.patch17.17 KBAlan Evans
PASSED: [[SimpleTest]]: [MySQL] 54,011 pass(es).
[ View ]
#7 interdiff.txt4.23 KBAlan Evans
#3 drupal-book-admin-edit-1945390-3.patch16.39 KBAlan Evans
PASSED: [[SimpleTest]]: [MySQL] 53,247 pass(es).
[ View ]

Comments

Looking into this one...

Assigned:Unassigned» Alan Evans

StatusFileSize
new16.39 KB
PASSED: [[SimpleTest]]: [MySQL] 53,247 pass(es).
[ View ]

First stab at this attached ...

So, one thing to note: the regex on node in the book_admin_edit route is required in order to allow the /settings path still to match, otherwise the 2 routes compete. I think there's probably another way of doing the same thing, but this seemed a reasonable start, and works.

Looking at OutlineAccessCheck, it almost seems like we want a more generic access check that can take in _requirements the name of a permission and a handful of node access arguments so we could reuse the same access check on various routes - you'd just specify the variable bits in the route _requirements (so, typically, you'd specify a permission and a number of node access ops). Does that make sense? Might give that a try next before moving the file elsewhere. It just seems like a fairly common need to have an access check that checks a permission and node access.

Status:Active» Needs review

Status:Needs review» Needs work

+++ b/core/modules/book/lib/Drupal/book/Access/OutlineAccessCheck.php
@@ -0,0 +1,32 @@
+class OutlineAccessCheck implements AccessCheckInterface {
+
+  /**
+   * Implements AccessCheckInterface::applies().
+   */
+  public function applies(Route $route) {
+    return array_key_exists('_book_outline_access', $route->getRequirements());
+  }
+
+  /**
+   * Implements AccessCheckInterface::access().
+   */
+  public function access(Route $route, Request $request) {
+    return user_access('administer book outlines') && node_access('view', $request->get('node'));
+  }

Unfortunately, there's no && for stacking route permissions at the moment, only ||. That means we can't entirely get rid of this custom checker.

However, what could be good is to make the PermissionAccessCheck a dependency of this service, and then subcall it. That is:

$this->permissionCheck->access($route, $request)

That replaces user_access(). Of course, now looking at PermissionAccessCheck it insists on taking the permission from the route. So... I am not sure if it's legal to add an optional $permission parameter to PermissionAccessCheck::access() to use in place of what's on the route. Hm. Maybe I'm over-thinking this. :-)

Whatever we decide to do there should also be done with the node_access() call.

Also, $request->get('node') is probably wrong. I think you mean $request->attributes->get('node')

And actually, never mind. We need to just turn user_access() and node_access() into services that get injected to this object. That's all we need to do. Ignore everything I wrote above. :-)

+++ b/core/modules/book/lib/Drupal/book/Form/BookAdminEditForm.php
@@ -0,0 +1,198 @@
+      cache('menu')->delete($cid);

The cache is now injectable, I think, so let's inject it.

+++ b/core/modules/book/lib/Drupal/book/Form/BookAdminEditForm.php
@@ -0,0 +1,198 @@
+  private function book_admin_table(EntityInterface $node, &$form) {

This should be protected. Drupal standards say to never use private. Also, lowerCamel name.

+++ b/core/modules/book/lib/Drupal/book/Form/BookAdminEditForm.php
@@ -0,0 +1,198 @@
+    $tree = book_menu_subtree_data($node->book);

Perhaps this method could move to the BookManager class once #1938296: Convert book_admin_overview and book_render to a new-style Controller finally gets committed.

+++ b/core/modules/book/lib/Drupal/book/Form/BookAdminEditForm.php
@@ -0,0 +1,198 @@
+      $this->book_admin_table_tree($tree['below'], $form['table']);

Method names should be lowerCamel. So $this->adminTableTree().

+++ b/core/modules/book/lib/Drupal/book/Form/BookAdminEditForm.php
@@ -0,0 +1,198 @@
+  private function book_admin_table_tree($tree, &$form) {

Method names should be lowerCamel, and protected, not private.

Thanks for the brilliant review, @Crell. Minor changes done, looking into the injection changes (cache, user_access and node_access) ...

Status:Needs work» Needs review
StatusFileSize
new4.23 KB
new17.17 KB
PASSED: [[SimpleTest]]: [MySQL] 54,011 pass(es).
[ View ]

Progress patch:

  • cache menu is injected
  • method access changed to protected
  • methods renamed
  • changed to using ->attributes->get ... it turns out this is a fairly subtle point: you can use Request::get, but you shouldn't. It works, but is not very specific and has multiple downsides

I think there are a couple of comments needed still surrounding the function name changes here.

Injecting user_access and node_access was not quite as simple as I'd thought, so I'll post more on that when I have a moment.

I'll just set this in review to get it tested and then back to needs work ...

Status:Needs review» Needs work

Ignore node_access() and user_access() for the moment. Those should get their own issues.

Well, looks like I totally ballsed the last patch up, but other than that I'm sure it's fine ...

(investigating ...)

Status:Needs work» Needs review

No idea, all green locally - test logs look weird (lots of file writing errors) ... rerolling against latest 8.x showed no differences. Requeueing for now.

StatusFileSize
new17.85 KB
PASSED: [[SimpleTest]]: [MySQL] 53,854 pass(es).
[ View ]
new1.09 KB

Those minor comment corrections ...

Status:Needs review» Needs work

+++ b/core/modules/book/book.routing.yml
@@ -4,3 +4,11 @@ book_settings:
+    node: \d+

We haven't been doing much of that, but classy. :-)

+++ b/core/modules/book/lib/Drupal/book/Form/BookAdminEditForm.php
@@ -0,0 +1,225 @@
+   * @see book_admin_edit()

This @see is now out of date.

I think that's the only thing left I could find.

Great, thanks.

Was tempted to just remove it rather than adding a somewhat redundant-looking pointer, but I guess it's handy to have in the api docs.

Status:Needs work» Needs review
StatusFileSize
new1.01 KB
new17.91 KB
PASSED: [[SimpleTest]]: [MySQL] 53,995 pass(es).
[ View ]

Status:Needs review» Needs work

#1939660: Use YAML as the primary means for service registration went in, you'll have to make it a book.services.yml now instead of BookBundle.

Thanks - on it.

Status:Needs work» Needs review
StatusFileSize
new17.47 KB
PASSED: [[SimpleTest]]: [MySQL] 54,192 pass(es).
[ View ]
new1.15 KB

StatusFileSize
new17.44 KB
PASSED: [[SimpleTest]]: [MySQL] 54,151 pass(es).
[ View ]
new1.13 KB

whitespace correction

Following on from #5 and #9, I've created an issue and taken a first pass at converting user_access to an injectable service (just roughly converted a single test for the time being)

@Crell, @tim.plunkett - if you have time, could you take a look and check if the direction looks sane and if so I'll put some more work into it. Thanks!

... and that issue is at #1966334: Convert user_access to User::hasPermission().

I'm also following up on a similar approach to node_access (just using a simple wrapper class to make it injectable), will probably create an issue for that too shortly.

Just in case you missed it, there is http://drupalcode.org/project/drupal.git/blob/refs/heads/8.x:/core/modul...
However, access checks are not currently AND-able, only OR, so it doesn't help too much.

I had seen that while looking for usable access solutions for this issue, yes, but as you mention it only does OR, so that pretty much ruled it out. The other downside is that it always takes its arguments from the route, so you can't use it out of that context either.

Experimental issue for making node_access a service is here: #1966760: Convert node_access to an injectable service wrapper of some sort

+++ b/core/modules/book/book.moduleundefined
@@ -453,7 +450,7 @@ function _book_add_form_elements(&$form, &$form_state, EntityInterface $node) {
+  // @see \Drupal\book\Form\BookAdminEditForm::bookAdminTableTree(). The weight may be larger than 15.

exceeds 80 char limit

+++ b/core/modules/book/lib/Drupal/book/Access/OutlineAccessCheck.phpundefined
@@ -0,0 +1,32 @@
+   * Implements AccessCheckInterface::applies().
...
+   * Implements AccessCheckInterface::access().

AccessCheckInterface needs the full namespaced path..but thank go we can now use inheritdocs
#1906474: [policy adopted] Stop documenting methods with "Overrides …"

Status:Needs review» Needs work

Rerolling and integrating these ...

Status:Needs work» Needs review
StatusFileSize
new17.59 KB
PASSED: [[SimpleTest]]: [MySQL] 55,263 pass(es).
[ View ]
new1.84 KB

(in the most recent patch also removed one redundant "Use" which, ironically, was not used).

looking really good! nice job:)

+++ b/core/modules/book/lib/Drupal/book/Form/BookAdminEditForm.phpundefined
@@ -0,0 +1,225 @@
+   * Implements \Drupal\Core\ControllerInterface::create().
...
+   * Implements \Drupal\Core\Form\FormInterface::getFormID().
...
+   * Implements \Drupal\Core\Form\FormInterface::validateForm().
+   *
+   * Checks that the book has not been changed while using the form.
...
+  /**
+   * Implements \Drupal\Core\Form\FormInterface::submitForm().
+   *
+   * This function takes care to save parent menu items before their children.
+   * Saving menu items in the incorrect order can break the menu tree.
+   *
+   * @see menu_overview_form_submit()
+   */

I guess we should make those inheritdocs too, sorry forgot to mention it above

+++ b/core/modules/book/lib/Drupal/book/Form/BookAdminEditForm.phpundefined
@@ -0,0 +1,225 @@
+    $tree = array_shift($tree); // Do not include the book item itself.

And maybe since you are there move this inline comment above the statement:)

Otherwise this looks good to go if bot agrees

StatusFileSize
new17.38 KB
PASSED: [[SimpleTest]]: [MySQL] 55,820 pass(es).
[ View ]
new2.02 KB

Thanks for the reviews! Good point on all those "implements". I've included those changes as described. It seems like {@inheritdoc} should play nicely with any other comments present - the phpdoc website indicates that it merges "parent" comments in at the point the tag appears in comments.

WOOHOOOOO I got interdiff file number 5000!! Is there a prize?

hehehe:) nice
The thing with inheritdoc, is that it has to be alone to work
So while this works:

/**
* {@inheritdoc}
*/

This won't:

/**
* {@inheritdoc}
*
* One extra line from info.
*/

It wont be parsed from api module. So, lets remove those extra lines, i dont see anything worth saving

Status:Needs review» Needs work

Alan Evans, are you still working on this?

Sorry, work took over for a bit - yes, I have a bit of time today. Seems like just a couple of tweaks left and probably a reroll by now also..

Rebase was moderately painful, so I'll be posting an unchanged patch for the bot as soon as a quick check of book tests completes locally. Oddly the more painful parts were non-conflicting changes that needed tracking down (the conflicts sometimes make things clearer to fix up, where an absence of conflicts can make it easy to miss important upstream changes). Think I'm about there though ...

Status:Needs work» Needs review
StatusFileSize
new17.57 KB
PASSED: [[SimpleTest]]: [MySQL] 56,964 pass(es).
[ View ]

Book tests passed locally, so here goes ... As mentioned, this one's just for the bot for now (doesn't include any changes from the previous, just a rebase). I think I did a reasonable job of pulling upstream changes into the right place in this patch. We'll see if the bot can confirm that or not.

Next patch then hopefully will be the final tweaks to comments etc.

Status:Needs review» Needs work

Status:Needs work» Needs review
StatusFileSize
new17.16 KB
PASSED: [[SimpleTest]]: [MySQL] 57,419 pass(es).
[ View ]
new1.47 KB

Another rebase, this time including removing additional comments from all the inheritdocs, which I think was the last review point that needed doing. Hopefully got all those points in now.

Many thanks once again for the reviews!

Status:Needs review» Needs work

This is really good so far!

+++ b/core/modules/book/book.moduleundefined
@@ -173,12 +174,8 @@ function book_menu() {
   $items['admin/structure/book/%node'] = array(
     'title' => 'Re-order book pages and change titles',
-    'page callback' => 'drupal_get_form',
-    'page arguments' => array('book_admin_edit', 3),
-    'access callback' => '_book_outline_access',
-    'access arguments' => array(3),
+    'route_name' => 'book_admin_edit',
     'type' => MENU_CALLBACK,
-    'file' => 'book.admin.inc',

If you just have a MENU_CALLBACK you can drop it entirely.

+++ b/core/modules/book/lib/Drupal/book/Access/OutlineAccessCheck.phpundefined
@@ -0,0 +1,32 @@
+ * Contains Drupal\book\Access\OutlineAccessCheck.

Should be "Contains \"...

+++ b/core/modules/book/lib/Drupal/book/Access/OutlineAccessCheck.phpundefined
@@ -0,0 +1,32 @@
+    return user_access('administer book outlines') && node_access('view', $request->attributes->get('node'));

You want to return NULL if your two checks don't apply. Can't you also call $node->access('view') directly?

+++ b/core/modules/book/lib/Drupal/book/Form/BookAdminEditForm.phpundefined
@@ -0,0 +1,216 @@
+          $menu_link = entity_load('menu_link', $values['mlid']);

Instead of calling entity load you could inject the entity manager and from there get the storage controller for the menu_link entity type.

+++ b/core/modules/book/lib/Drupal/book/Form/BookAdminEditForm.phpundefined
@@ -0,0 +1,216 @@
+          $node = node_load($values['nid']);

That's a place where you could use load on the node storage controller. This might be a place where you could use load($nids) so leverage the multiple-load capability.

We could get rid of the access checker entirely if #1987768: Convert node_page_view() to a new style controller is in.

Status:Needs work» Needs review
StatusFileSize
new5.42 KB
new17.6 KB
PASSED: [[SimpleTest]]: [MySQL] 58,397 pass(es).
[ View ]

reroll plus changes in #41, plus switched to using FormBase.

Assigned:Alan Evans» disasm

+++ b/core/modules/book/lib/Drupal/book/Form/BookAdminEditForm.php
@@ -93,7 +105,7 @@ public function submitForm(array &$form, array &$form_state) {
+          $menu_link = $this->entityManager->getStorageController('menu_link')->load($values['mlid']);
@@ -102,12 +114,12 @@ public function submitForm(array &$form, array &$form_state) {
+          $node = $this->entityManager->getStorageController('node')->load($values['nid']);

In general, and especially here because they are in a foreach loop, you should get the storage controllers in create(). That way __construct() can have the proper typehints

StatusFileSize
new3.46 KB
new18.23 KB
PASSED: [[SimpleTest]]: [MySQL] 58,353 pass(es).
[ View ]

+++ b/core/modules/book/lib/Drupal/book/Form/BookAdminEditForm.php
@@ -29,30 +29,46 @@ class BookAdminEditForm extends FormBase implements ContainerInjectionInterface
+   * @var \Drupal\Core\Entity\EntityStorageControllerInterface
...
+  public function __construct(CacheBackendInterface $cache, EntityStorageControllerInterface $node_storage, EntityStorageControllerInterface $menu_link_storage) {

Not sure why NodeStorageController doesn't have an interface, but MenuLinkStorageControllerInterface exists.

StatusFileSize
new1.84 KB
new18.29 KB
PASSED: [[SimpleTest]]: [MySQL] 58,438 pass(es).
[ View ]

Using MenuLinkStorageControllerInterface in AdminEditForm

  1. +++ b/core/modules/book/book.module
    @@ -175,12 +176,8 @@ function book_menu() {
       $items['admin/structure/book/%node'] = array(
         'title' => 'Re-order book pages and change titles',
    -    'page callback' => 'drupal_get_form',
    -    'page arguments' => array('book_admin_edit', 3),
    -    'access callback' => '_book_outline_access',
    -    'access arguments' => array(3),
    +    'route_name' => 'book_admin_edit',
         'type' => MENU_CALLBACK,
    -    'file' => 'book.admin.inc',
       );

    Just put a _title property unto the route defaults and you can remove the hook_menu entry as it is.

  2. +++ b/core/modules/book/lib/Drupal/book/Access/OutlineAccessCheck.php
    @@ -0,0 +1,32 @@
    +    return user_access('administer book outlines') && node_access('view', $request->attributes->get('node'));

    Let's replace that with current_user in the future.

  3. +++ b/core/modules/book/lib/Drupal/book/Form/BookAdminEditForm.php
    @@ -0,0 +1,245 @@
    +    drupal_set_message(t('Updated book %title.', array('%title' => $form['#node']->label())));

    This one should also use $this->t()

  4. +++ b/core/modules/book/lib/Drupal/book/Form/BookAdminEditForm.php
    @@ -0,0 +1,245 @@
    +   * @param \Drupal\Core\Entity\EntityInterface $node
    +   *   The node of the top-level page in the book.
    ...
    +  protected function bookAdminTable(EntityInterface $node, &$form) {

    Do you think we can typehint on NodeInterface instead?

StatusFileSize
new2.18 KB
new18.48 KB
PASSED: [[SimpleTest]]: [MySQL] 58,437 pass(es).
[ View ]

reroll plus 1 and 3 from #49. For 2, would love to once it gets in, and 4, I'm not sure. tim.plunkett can probably clarify.

Status:Needs review» Needs work

+++ b/core/modules/book/lib/Drupal/book/Form/BookAdminEditForm.php
@@ -0,0 +1,245 @@
+    $tree = book_menu_subtree_data($node->book);

Can we convert this to OO.

Status:Needs work» Needs review

Lets do #51 in a follow-up. As I stated in #1938314: Convert book_export to a new-style Controller, which also depends on book_menu_subtree_data, and webchick agreed, we want to get as many of these that are passing committed and we can handle refactoring in follow-ups. Right now the clock is ticking until we change gears on these conversions. The way this is now is better than the alternative.

Status:Needs review» Reviewed & tested by the community

Ok then it is ready to fly.

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new16.69 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch book-1945390-54.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new7.11 KB

I noticed a couple oddities while looking over the final patch.

Ideally this would have been an entity form controller, but it might be too late for that?

Status:Needs review» Needs work

This either needs quick converted, or if it's not too difficult, switched to entity form controller as decided in WSCCI weekly meeting. Any case, we don't want this landing before book_outline because it's becoming a bear to reroll.

Thanks for your work on this issue! Please see #1971384-43: [META] Convert page callbacks to controllers for an update on the routing system conversion process.

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

#54: book-1945390-54.patch queued for re-testing.

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

The last submitted patch, book-1945390-54.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new16.87 KB
PASSED: [[SimpleTest]]: [MySQL] 58,793 pass(es).
[ View ]

+++ b/core/modules/book/lib/Drupal/book/Form/BookAdminEditForm.php
@@ -0,0 +1,234 @@
+
+  /**
+   * {@inheritdoc}
+   */
+  public function buildForm(array $form, array &$form_state, NodeInterface $node = NULL) {

Have we figured out any kind of standard for this optional parameters?

This is just a rerole.

Back to RTBC. I am ignoring #55. We can do it in follow up issue.

Status:Needs review» Reviewed & tested by the community

Status:Reviewed & tested by the community» Fixed

Committed and pushed to 8.x. Thanks!

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

Issue summary:View changes

Change records don't link.