Convert book_remove_form to a new-style Form object

mine

Grabbing this from @disasm.

	modified:   book.module
	modified:   book.pages.inc
	new file:   book.routing.yml
	new file:   lib/Drupal/book/Access/BookRemoveAccessCheck.php
	new file:   lib/Drupal/book/BookBundle.php
	new file:   lib/Drupal/book/Form/BookRemoveForm.php

Note that BookRemoveAccessCheck::access() is still calling _book_outline_access() here, which will likely be removed and replaced by other patches during the sprint. We'll need to revisit this later.

disasm suggested splitting the access checks in requirements into atomic pieces. Here's the old access callback and its 2 dependent functions:

function _book_outline_remove_access(Node $node) {
  return _book_node_is_removable($node) && _book_outline_access($node);
}

function _book_node_is_removable($node) {
  return (!empty($node->book['bid']) && (($node->book['bid'] != $node->nid) || !$node->book['has_children']));
}

function _book_outline_access(Node $node) {
  return user_access('administer book outlines') && node_access('view', $node);
}

The new patch replaces these with the following routing.yml requirements:

  requirements:
    _permission: 'administer book outlines'
    _book_node_access: 'view'
    _book_node_is_removable: 'TRUE'

The "_book_node_access" check should probably belong to node module, but I was leery about including something outside of book module scope in this patch.

Here's what's inside:

#	modified:   book.module
#	modified:   book.pages.inc
#	new file:   book.routing.yml
#	new file:   lib/Drupal/book/Access/BookNodeAccessCheck.php
#	new file:   lib/Drupal/book/Access/BookNodeIsRemovableAccessCheck.php
#	new file:   lib/Drupal/book/BookBundle.php
#	new file:   lib/Drupal/book/Form/BookRemoveForm.php

Removed unnecessary service retrieval.

This patch looks awesome! The only comment I have is to use the DIC for the database transaction. Also, your comments are very nice and detailed stating what your doing, but creating an interdiff for changes is even nicer for reviewers! If it's just one commit difference, it's as simple as git diiff HEAD~ > interdiff.txt. If it's more than one commit, you should microbranch your changes, otherwise, you have to find the last commit and git diff > interdiff.txt.

+++ b/core/modules/book/lib/Drupal/book/Form/BookRemoveForm.phpundefined
@@ -0,0 +1,89 @@
+      db_delete('book')

replace with $this->database->delete

Lovely! New patch.

No, testbot! do this one.

Poop, I didn't see Crell's review before that latest patch. Will setting "needs work" stop the testbot?

In addition to Crell's comments above, I don't know how we both missed this in our reviews above.

+++ b/core/modules/book/book.moduleundefined
@@ -188,11 +188,7 @@ function book_menu() {
+    'route' => 'book_remove',

should be route_name

New patch to accommodate Crell's review in #10 and disasm in #14.

Was going to go to sleep, somehow ended up pulling on 8.x and re-rolling.

Just a note, 1925196 hit a few minutes ago, so you'll need to reroll in the morning!

#16: 1938318-convert-book_remove_form-routing-16.patch queued for re-testing.

Nope, #1925196: Convert book's system_config_form() to SystemConfigFormBase was already there when I pulled 8.x last night. #16 is current.

x-post.

Re-rerolled after Node -> EntityNG conversion.

wait, I think the typehinting for Node needs to be changed now.

This should be good.

Tagging. Didn't review.

#16: 1938318-convert-book_remove_form-routing-16.patch queued for re-testing.

For reasons I don't understand, the route pattern showed up in the Tools menu. Removed the item's entry in book_menu(), since we only need the route anyway.

Since this is not a MENU_CALLBACK, I'm very wary of removing it entirely. That feels like it's going to cause more issues with the menu.

And that's fine, I can put the item back in book_menu(). I think we just need an access check to verify that {node} is actually a valid node. That ought to be a part of BookNodeIsRemovableAccessCheck::access(), anyway.

Er, wait a sec. The YAML comes back as "TRUE" or "FALSE", not as booleans? That smells funny to me...

The parser will only accept a string value in the YAML routing file. If I try a boolean or even an integer, I get this:

InvalidArgumentException: Routing requirement for "_book_node_is_removable" must be a string. in Symfony\Component\Routing\Route->sanitizeRequirement() (line 570 of /xxx/d8/core/vendor/symfony/routing/Symfony/Component/Routing/Route.php).

Actually, shouldn't we just return the value of node_access()? If node_access() says a user should be able to view this node, we should return true, shouldn't we?

In principle, I'd like to agree. But for "book remove" access, definitely not. This is where not being able to "and" access checks together is a problem. "administer book outlines" permission is the only part of this particular route's access stack that should be able to return TRUE. Neither "_node_access" nor "_book_node_is_removable" should be able to grant access here; they are only there to deny access.

I suppose in most cases, there is some permission that must be present to allow access to a particular route. If so, any other access check in the stack should not be allowed to return TRUE unless it is meant to bypass that permission.

Now that we have #1938600: Add a FormInterface replacement for confirm_form(), BookRemoveForm should extend ConfirmFormBase.

Double blargh on YAML. That bites.

This is a Symfony limitation on requirements. This method comes from Route.php:

    private function sanitizeRequirement($key, $regex)                               
    {                                                                                
        if (!is_string($regex)) {                                                    
            throw new \InvalidArgumentException(sprintf('Routing requirement for "%s" must be a string.', $key));
        }                                                                            
                                                                                     
        if ('' !== $regex && '^' === $regex[0]) {                                    
            $regex = (string) substr($regex, 1); // returns false for a single character
        }                                                                            
                                                                                     
        if ('$' === substr($regex, -1)) {                                            
            $regex = substr($regex, 0, -1);                                          
        }                                                                            
                                                                                     
        if ('' === $regex) {                                                         
            throw new \InvalidArgumentException(sprintf('Routing requirement for "%s" cannot be empty.', $key));
        }                                                                            
                                                                                     
        // this is to keep BC and will be removed in a future version                
        if ('_scheme' === $key) {                                                    
            $this->setSchemes(explode('|', $regex));                                 
        } elseif ('_method' === $key) {                                              
            $this->setMethods(explode('|', $regex));                                 
        }                                                                            
                                                                                     
        return $regex;                                                               
    }                                           

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

#30: 1938318-convert-book_remove_form-routing-30.patch queued for re-testing.

restoring status

This now uses services YAML syntax.

Besides the need of {@inheritdocs} in the patch above, it should extend ConfirmFormBase see #35

Also can we move _book_node_is_removable() logic somewhere? after this conversion there will be no point keeping it as procedural access check

+++ b/core/modules/book/book.moduleundefined
@@ -180,14 +180,6 @@ function book_menu() {
-  $items['node/%node/outline/remove'] = array(
-    'title' => 'Remove from outline',
-    'page callback' => 'drupal_get_form',

Did this have a visible menu entry or breadcrumbs before? If so, this is still needed.

+++ b/core/modules/book/book.routing.ymlundefined
@@ -18,3 +18,12 @@ book_settings:
+    _permission: 'administer book outlines'
+    _node_access: 'view'
+    _book_node_is_removable: 'TRUE'

This doesn't work as you expect. These are OR'd together, not AND'd. So anyone with view access will get in, regardless of the other two access checkers. This is a big problem, we should fix this in a separate issue.

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

+++ b/core/modules/book/lib/Drupal/book/Form/BookRemoveForm.phpundefined
@@ -0,0 +1,92 @@
+   * Implements \Drupal\Core\Form\FormInterface::getFormID().
...
+   * Implements \Drupal\Core\Form\FormInterface::validateForm().
...
+   * Implements \Drupal\Core\Form\FormInterface::submitForm().

+++ b/core/modules/node/lib/Drupal/node/Access/NodeAccessCheck.phpundefined
@@ -0,0 +1,36 @@
+   * Implements AccessCheckInterface::applies().
...
+   * Implements AccessCheckInterface::access().

{@inheritdoc}

+++ b/core/modules/book/lib/Drupal/book/Access/BookNodeIsRemovableAccessCheck.phpundefined
@@ -0,0 +1,36 @@
+    $removable = (_book_node_is_removable($node) ? 'TRUE' : 'FALSE');
+    if ($removable != strtoupper($route->getRequirement('_book_node_is_removable'))) {

Is this actually needed? We should find a way to just use booleans

+++ b/core/modules/book/lib/Drupal/book/Form/BookRemoveForm.phpundefined
+++ b/core/modules/book/lib/Drupal/book/Form/BookRemoveForm.phpundefined
@@ -0,0 +1,92 @@

@@ -0,0 +1,92 @@
+<?php
+namespace Drupal\book\Form;

Missing blank line and @file block

+++ b/core/modules/book/lib/Drupal/book/Form/BookRemoveForm.phpundefined
@@ -0,0 +1,92 @@
+class BookRemoveForm implements ControllerInterface, FormInterface {

Missing docblock

+++ b/core/modules/book/lib/Drupal/book/Form/BookRemoveForm.phpundefined
@@ -0,0 +1,92 @@
+   * Stores a \Drupal\Core\Database\Connection instance.

missing @var

+++ b/core/modules/book/lib/Drupal/book/Form/BookRemoveForm.phpundefined
@@ -0,0 +1,92 @@
+   * @param \Symfony\Component\DependencyInjection\ContainerInterface

missing $container

+++ b/core/modules/book/lib/Drupal/book/Form/BookRemoveForm.phpundefined
@@ -0,0 +1,92 @@
+   * @param Drupal\Core\Entity\EntityInterface $node

@param \Drupal\...

+++ b/core/modules/book/lib/Drupal/book/Form/BookRemoveForm.phpundefined
@@ -0,0 +1,92 @@
+   * @see book_menu()
+   * @see \Drupal\book\Form\BookRemoveForm::submitForm()
...
+   * @see book_menu()
+   * @see \Drupal\book\Form\BookRemoveForm::buildForm()

Remove this

+++ b/core/modules/book/lib/Drupal/book/Form/BookRemoveForm.phpundefined
@@ -0,0 +1,92 @@
+    $form['#node'] = $node;
...
+    $node = $form['#node'];

Set $this->node instead

Refactored to use confirm form, replaced with new NodeInterface, and fixed comments as per #43.

OK. NodeInterface may have been a bit ambitious. I also noticed a few other issues such as buildForm() not returning parent buildForm() and a t function around confirmText().

This is failing because BookNodeIsRemovableAccessCheck::access() gets called twice.

The first time from AccessSubscriber::onKernelRequestAccessCheck().

The second time 'node' isn't in the request attributes when called from menu.inc:903 _menu_link_translate().

Testing something out. If this passes, we have another bug.

Ensure the node is in the request attributes before calling _book_node_is_removable()

#51: 1938318-book-remove-form-interface-51.patch queued for re-testing.

I am working on this.
I am going to move _book_node_is_removable to Drupal\book\BookManager.
I am going to create Drupal\book\BookManager::deleteBook($nid) to remove.

+++ b/core/modules/book/lib/Drupal/book/Form/BookRemoveForm.phpundefined
@@ -0,0 +1,120 @@
+      $this->database->delete('book')
+        ->condition('nid', $node->nid)

I think node.services.yml changes are not required because

+++ b/core/modules/book/book.routing.ymlundefined
@@ -18,3 +18,12 @@ book_settings:
+    _node_access: 'view'

can be replaced with _entity_access: node.view

+++ b/core/modules/book/book.routing.ymlundefined
@@ -18,3 +18,12 @@ book_settings:
+book_remove:
+  pattern: 'node/{node}/outline/remove'
+  defaults:
+    _form: '\Drupal\book\Form\BookRemoveForm'
+  requirements:

This options have been connected with && before, so you have to set this on the route definition, _access_checks: 'ALL' is the option you need.

+++ b/core/modules/book/book.routing.ymlundefined
@@ -18,3 +18,12 @@ book_settings:
+    _node_access: 'view'

Yes that is right, there is no need for that, just use entity_access.

+++ b/core/modules/book/lib/Drupal/book/Access/BookNodeIsRemovableAccessCheck.phpundefined
@@ -0,0 +1,38 @@
+        return FALSE;

You should return static::DENY here probably instead of return FALSE;

+++ b/core/modules/book/lib/Drupal/book/Form/BookRemoveForm.phpundefined
@@ -0,0 +1,120 @@
+   * @param \Drupal\Core\Entity\EntityInterface $node
+   *   The node to delete.
...
+  public function buildForm(array $form, array &$form_state, EntityInterface $node = NULL) {

It is a node, so let's type hint it more specific

+++ b/core/modules/book/lib/Drupal/book/Form/BookRemoveForm.phpundefined
@@ -0,0 +1,120 @@
+      menu_link_delete($node->book['mlid']);

Menu link delete should be replaced by the menu link storage controller ...

+++ b/core/modules/book/lib/Drupal/book/Form/BookRemoveForm.phpundefined
@@ -0,0 +1,120 @@
+    $form_state['redirect'] = 'node/' . $node->nid;

You have the full node available so maybe using $node->uri() is the way to to it

+++ b/core/modules/node/lib/Drupal/node/Access/NodeAccessCheck.phpundefined
@@ -0,0 +1,36 @@
+class NodeAccessCheck implements AccessCheckInterface {

This would 100% conflict with the NodeAccessCheck which is currently in core (which cares about permission).

+++ b/core/modules/node/lib/Drupal/node/Access/NodeAccessCheck.phpundefined
@@ -0,0 +1,36 @@
+    if (!node_access($op, $node)) {

Are you sure you don't want to have the logic of node_access, which does a little bit more then the node access controller?

Implemented #55 and fixed #56. Thanks @dawehner for the great review.

Some silly fixes.

#59: 1938318-59.patch queued for re-testing.

_entity_access: 'node.view' has some problem in test it always returns false. Manual testing has shown that patch is working fine. I am also uploading a pass patch.

:/

This is blocked by #1947880: Replace node_access() by $entity->access()

This needs a reroll for FormBase, and is still blocked.

Started with a straight reroll, and then updated BookRemoveForm for changes in FormBase. Still blocked on #1947880: Replace node_access() by $entity->access().

Can you add a pass patch as well?

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.

Updated issue summary.

This is the second-to-last confirm_form. No interdiff because its just been too long.

menu_link_delete() is more complex still, we need to continue to use that.

+++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkStorageControllerInterface.php
@@ -13,7 +13,7 @@
-interface MenuLinkStorageControllerInterface {
+interface MenuLinkStorageControllerInterface extends EntityStorageControllerInterface {

Wow, good catch!!

Not sure why this is major, since it's not blocking a major. Nevertheless..

Committed and pushed to 8.x. Thanks!

Sorry for the late re-open, happy to close and open a follow-up but wanted to ask first.

That MenuLinkStorageControllerInterface was explicitly like that, because it causes crazy bugs, see #2095399-14: Merge DatabaseStorageController and DatabaseStorageControllerNG, #17 there and the broken HEAD that we had in https://drupal.org/node/731724?page=1 (#527), which links to https://bugs.php.net/bug.php?id=49472.

Crell was just fighting with that...

Ah whatever, I think this only affects some weird 5.3.x versions, which we no longer support, so let's just forget about it...