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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Alan Evans’s picture

Looking into this one...

Alan Evans’s picture

Assigned: Unassigned » Alan Evans
Alan Evans’s picture

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.

Alan Evans’s picture

Status: Active » Needs review
Crell’s picture

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.

Alan Evans’s picture

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

Alan Evans’s picture

Status: Needs work » Needs review
FileSize
4.23 KB
17.17 KB

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

Alan Evans’s picture

Status: Needs review » Needs work
Crell’s picture

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

Alan Evans’s picture

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

(investigating ...)

Alan Evans’s picture

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.

Alan Evans’s picture

Alan Evans’s picture

Those minor comment corrections ...

Crell’s picture

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.

Alan Evans’s picture

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.

Alan Evans’s picture

Status: Needs work » Needs review
FileSize
1.01 KB
17.91 KB
tim.plunkett’s picture

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.

Alan Evans’s picture

Thanks - on it.

Alan Evans’s picture

Status: Needs work » Needs review
FileSize
17.47 KB
1.15 KB
Alan Evans’s picture

whitespace correction

Alan Evans’s picture

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!

Alan Evans’s picture

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

tim.plunkett’s picture

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.

Alan Evans’s picture

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

ParisLiakos’s picture

+++ 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 …"

ParisLiakos’s picture

Status: Needs review » Needs work
Alan Evans’s picture

Rerolling and integrating these ...

Alan Evans’s picture

Status: Needs work » Needs review
FileSize
17.59 KB
1.84 KB
Alan Evans’s picture

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

ParisLiakos’s picture

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

Alan Evans’s picture

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.

Alan Evans’s picture

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

ParisLiakos’s picture

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

ParisLiakos’s picture

Status: Needs review » Needs work
kim.pepper’s picture

Alan Evans, are you still working on this?

Alan Evans’s picture

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

Alan Evans’s picture

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

Alan Evans’s picture

Status: Needs work » Needs review
FileSize
17.57 KB

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.

Alan Evans’s picture

Status: Needs review » Needs work
Alan Evans’s picture

Status: Needs work » Needs review
FileSize
17.16 KB
1.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!

dawehner’s picture

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.

dawehner’s picture

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

disasm’s picture

Status: Needs work » Needs review
FileSize
5.42 KB
17.6 KB

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

disasm’s picture

Assigned: Alan Evans » disasm
tim.plunkett’s picture

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

disasm’s picture

tim.plunkett’s picture

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

disasm’s picture

Using MenuLinkStorageControllerInterface in AdminEditForm

dawehner’s picture

  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?

disasm’s picture

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.

jibran’s picture

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.

disasm’s picture

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.

jibran’s picture

Status: Needs review » Reviewed & tested by the community

Ok then it is ready to fly.

tim.plunkett’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
16.69 KB
7.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?

disasm’s picture

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.

xjm’s picture

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.

jibran’s picture

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.

dawehner’s picture

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

jibran’s picture

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

jibran’s picture

Status: Needs review » Reviewed & tested by the community
webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

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

Anonymous’s picture

Issue summary: View changes

Change records don't link.