Problem/Motivation

Doing conversions of all hook_menu items to form objects and route controllers

Proposed resolution

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

Remaining tasks

Change BookRemoveForm to extend ConfirmFormBase #1938600: Add a FormInterface replacement for confirm_form()

CommentFileSizeAuthor
#73 interdiff.txt2.46 KBtim.plunkett
#73 book-1938318-73.patch8.96 KBtim.plunkett
#71 book-1938318-71.patch9.57 KBtim.plunkett
#67 drupal8.book-module.1938318-67.patch13.24 KBdisasm
#67 interdiff.txt3.53 KBdisasm
#63 1938318-63.patch13.09 KBjibran
#63 interdiff.txt2.44 KBjibran
#63 1938318-63-pass.patch13.09 KBjibran
#63 pass-interdiff.txt421 bytesjibran
#59 1938318-59.patch12.67 KBjibran
#59 interdiff.txt2.14 KBjibran
#57 1938318-57.patch12.48 KBjibran
#57 interdiff.txt11.09 KBjibran
#51 1938318-book-remove-form-interface-51.patch9.91 KBkim.pepper
#51 interdiff.txt951 byteskim.pepper
#49 book-remove-1938318-49.patch10.69 KBtim.plunkett
#49 interdiff.txt845 bytestim.plunkett
#46 1938318-book-remove-form-interface-46.patch9.87 KBkim.pepper
#46 interdiff.txt1.43 KBkim.pepper
#44 1938318-book-remove-form-interface-44.patch9.79 KBkim.pepper
#44 interdiff.txt6.55 KBkim.pepper
#41 drupal-convert-book_remove_form-routing-1938318-41.patch9.91 KBdisasm
#41 interdiff.txt2.38 KBdisasm
#30 1938318-convert-book_remove_form-routing-30.patch10.75 KBLes Lim
#30 interdiff-1938318-25-30.txt432 bytesLes Lim
#25 1938318-convert-book_remove_form-routing-25.patch10.73 KBLes Lim
#25 interdiff-1938318-23-25.txt1.17 KBLes Lim
#23 1938318-convert-book_remove_form-routing-23.patch10.7 KBLes Lim
#16 1938318-convert-book_remove_form-routing-16.patch10.63 KBLes Lim
#15 1938318-convert-book_remove_form-routing-15.patch10.49 KBLes Lim
#15 interdiff-1938318-11-15.txt4.99 KBLes Lim
#11 1938318-convert-book_remove_form-routing-10.patch9.79 KBLes Lim
#11 interdiff-1938318-8.txt1.66 KBLes Lim
#8 1938318-convert-book_remove_form-routing-8.patch9.54 KBLes Lim
#8 interdiff-1938318-7.txt1.27 KBLes Lim
#7 1938318-convert-book_remove_form-routing-7.patch9.77 KBLes Lim
#6 1938318-convert-book_remove_form-routing.patch8.32 KBLes Lim
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Crell’s picture

Issue tags: +WSCCI-conversion

.

DamienMcKenna’s picture

Issue tags: +SprintWeekend2013
disasm’s picture

Assigned: Unassigned » disasm

mine

disasm’s picture

Assigned: disasm » Unassigned
Les Lim’s picture

Assigned: Unassigned » Les Lim

Grabbing this from @disasm.

Les Lim’s picture

Status: Active » Needs review
FileSize
8.32 KB
	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.

Les Lim’s picture

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
Les Lim’s picture

Removed unnecessary service retrieval.

disasm’s picture

Status: Needs review » Needs work

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

Crell’s picture

+++ b/core/modules/book/lib/Drupal/book/Access/BookNodeAccessCheck.php
@@ -0,0 +1,36 @@
+/**
+ * Determines access to administer the requested node's book outline.
+ */
+class BookNodeAccessCheck implements AccessCheckInterface {

This definitely looks like it belongs in node module. A generic node access checker makes total sense to me. Moving this code to node module is totally fine to do in this patch if it makes sense.

+++ b/core/modules/book/lib/Drupal/book/Form/BookRemoveForm.php
@@ -0,0 +1,89 @@
+  protected $database;

Needs docblock.

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

This should be $this->database->delete()

Les Lim’s picture

Lovely! New patch.

No, testbot! do this one.

Les Lim’s picture

Status: Needs work » Needs review
Les Lim’s picture

Status: Needs review » Needs work

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

disasm’s picture

Status: Needs work » Needs review

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

Les Lim’s picture

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

Les Lim’s picture

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

Status: Needs review » Needs work

The last submitted patch, 1938318-convert-book_remove_form-routing-16.patch, failed testing.

disasm’s picture

Status: Needs work » Needs review

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

disasm’s picture

Crell’s picture

Status: Needs review » Reviewed & tested by the community

Apparently it didn't conflict.

Les Lim’s picture

Status: Reviewed & tested by the community » Needs review

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

Les Lim’s picture

Status: Needs review » Reviewed & tested by the community

x-post.

Les Lim’s picture

Re-rerolled after Node -> EntityNG conversion.

Les Lim’s picture

Status: Reviewed & tested by the community » Needs work

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

Les Lim’s picture

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

This should be good.

tim.plunkett’s picture

Issue tags: +FormInterface

Tagging. Didn't review.

Status: Reviewed & tested by the community » Needs work
Issue tags: -FormInterface, -SprintWeekend2013, -WSCCI-conversion

The last submitted patch, 1938318-convert-book_remove_form-routing-25.patch, failed testing.

disasm’s picture

Status: Needs work » Needs review
Issue tags: +FormInterface, +SprintWeekend2013, +WSCCI-conversion
Crell’s picture

Status: Needs review » Needs work

The bot still disagrees. :-(

Les Lim’s picture

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.

Les Lim’s picture

Status: Needs work » Needs review
Crell’s picture

Assigned: Les Lim » pwolanin
+++ b/core/modules/book/book.module
@@ -184,14 +184,6 @@ function book_menu() {
-  $items['node/%node/outline/remove'] = array(
-    'title' => 'Remove from outline',
-    'page callback' => 'drupal_get_form',
-    'page arguments' => array('book_remove_form', 1),
-    'access callback' => '_book_outline_remove_access',
-    'access arguments' => array(1),
-    'file' => 'book.pages.inc',
-  );

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.

I'm going to ping Peter Wolanin here, as this is still old-menu-system behavior, which he knows well, and he's also the book.module maintainer. :-)

+++ b/core/modules/book/lib/Drupal/book/Access/BookNodeIsRemovableAccessCheck.php
@@ -0,0 +1,36 @@
+  public function access(Route $route, Request $request) {
+    $node = $request->attributes->get('node');
+    $removable = (_book_node_is_removable($node) ? 'TRUE' : 'FALSE');
+    if ($removable != strtoupper($route->getRequirement('_book_node_is_removable'))) {
+      return FALSE;
+    }

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

+++ b/core/modules/node/lib/Drupal/node/Access/NodeAccessCheck.php
@@ -0,0 +1,36 @@
+  public function access(Route $route, Request $request) {
+    $node = $request->attributes->get('node');
+    $op = $route->getRequirement('_node_access');
+    if (!node_access($op, $node)) {
+      return FALSE;
+    }

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?

Les Lim’s picture

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.

Crell’s picture

Hm. Blargh on permission. In that case the node_access() call should be documented for why we're not just returning it as is.

Double blargh on YAML. That bites.

Peter, your thoughts?

tim.plunkett’s picture

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

Crell’s picture

Status: Needs review » Needs work
disasm’s picture

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;                                                               
    }                                           
tim.plunkett’s picture

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

disasm’s picture

Status: Needs work » Needs review
ParisLiakos’s picture

restoring status

disasm’s picture

This now uses services YAML syntax.

ParisLiakos’s picture

Status: Needs review » Needs work

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

tim.plunkett’s picture

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

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
6.55 KB
9.79 KB

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

Status: Needs review » Needs work

The last submitted patch, 1938318-book-remove-form-interface-44.patch, failed testing.

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
1.43 KB
9.87 KB

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

Status: Needs review » Needs work

The last submitted patch, 1938318-book-remove-form-interface-46.patch, failed testing.

kim.pepper’s picture

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

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
845 bytes
10.69 KB

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

Status: Needs review » Needs work

The last submitted patch, book-remove-1938318-49.patch, failed testing.

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
951 bytes
9.91 KB

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

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

The last submitted patch, 1938318-book-remove-form-interface-51.patch, failed testing.

jibran’s picture

Status: Needs work » Needs review

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

The last submitted patch, 1938318-book-remove-form-interface-51.patch, failed testing.

jibran’s picture

Assigned: pwolanin » jibran

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

dawehner’s picture

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

jibran’s picture

Status: Needs work » Needs review
FileSize
11.09 KB
12.48 KB

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

Status: Needs review » Needs work

The last submitted patch, 1938318-57.patch, failed testing.

jibran’s picture

Assigned: jibran » Unassigned
Status: Needs work » Needs review
FileSize
2.14 KB
12.67 KB

Some silly fixes.

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

The last submitted patch, 1938318-59.patch, failed testing.

jibran’s picture

Status: Needs work » Needs review

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

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

The last submitted patch, 1938318-59.patch, failed testing.

jibran’s picture

FileSize
421 bytes
13.09 KB
2.44 KB
13.09 KB

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

jibran’s picture

Status: Needs work » Needs review

:/

tim.plunkett’s picture

tim.plunkett’s picture

Status: Needs review » Needs work

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

disasm’s picture

Status: Needs work » Needs review
FileSize
3.53 KB
13.24 KB

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

Status: Needs review » Needs work

The last submitted patch, drupal8.book-module.1938318-67.patch, failed testing.

jibran’s picture

Can you add a pass patch as well?

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.

xjm’s picture

Issue summary: View changes

Updated issue summary.

tim.plunkett’s picture

Priority: Normal » Major
Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -SprintWeekend2013
FileSize
9.57 KB

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

Status: Needs review » Needs work

The last submitted patch, 71: book-1938318-71.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
8.96 KB
2.46 KB

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

dawehner’s picture

Status: Needs review » Reviewed & tested by the community
+++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkStorageControllerInterface.php
@@ -13,7 +13,7 @@
-interface MenuLinkStorageControllerInterface {
+interface MenuLinkStorageControllerInterface extends EntityStorageControllerInterface {

Wow, good catch!!

tim.plunkett’s picture

webchick’s picture

Priority: Major » Normal
Status: Reviewed & tested by the community » Fixed

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

Committed and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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

Berdir’s picture

Status: Closed (fixed) » Active

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

Berdir’s picture

Status: Active » Closed (fixed)

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