Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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()
Related Issues
Comment | File | Size | Author |
---|---|---|---|
#73 | interdiff.txt | 2.46 KB | tim.plunkett |
#73 | book-1938318-73.patch | 8.96 KB | tim.plunkett |
Comments
Comment #1
Crell CreditAttribution: Crell commented.
Comment #2
DamienMcKennaComment #3
disasm CreditAttribution: disasm commentedmine
Comment #4
disasm CreditAttribution: disasm commentedComment #5
Les LimGrabbing this from @disasm.
Comment #6
Les LimNote 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.
Comment #7
Les Limdisasm suggested splitting the access checks in requirements into atomic pieces. Here's the old access callback and its 2 dependent functions:
The new patch replaces these with the following routing.yml requirements:
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:
Comment #8
Les LimRemoved unnecessary service retrieval.
Comment #9
disasm CreditAttribution: disasm commentedThis 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.
replace with $this->database->delete
Comment #10
Crell CreditAttribution: Crell commentedThis 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.
Needs docblock.
This should be $this->database->delete()
Comment #11
Les LimLovely! New patch.
No, testbot! do this one.
Comment #12
Les LimComment #13
Les LimPoop, I didn't see Crell's review before that latest patch. Will setting "needs work" stop the testbot?
Comment #14
disasm CreditAttribution: disasm commentedIn addition to Crell's comments above, I don't know how we both missed this in our reviews above.
should be route_name
Comment #15
Les LimNew patch to accommodate Crell's review in #10 and disasm in #14.
Comment #16
Les LimWas going to go to sleep, somehow ended up pulling on 8.x and re-rolling.
Comment #18
disasm CreditAttribution: disasm commentedJust a note, 1925196 hit a few minutes ago, so you'll need to reroll in the morning!
Comment #19
disasm CreditAttribution: disasm commented#16: 1938318-convert-book_remove_form-routing-16.patch queued for re-testing.
Comment #20
Crell CreditAttribution: Crell commentedApparently it didn't conflict.
Comment #21
Les LimNope, #1925196: Convert book's system_config_form() to SystemConfigFormBase was already there when I pulled 8.x last night. #16 is current.
Comment #22
Les Limx-post.
Comment #23
Les LimRe-rerolled after Node -> EntityNG conversion.
Comment #24
Les Limwait, I think the typehinting for Node needs to be changed now.
Comment #25
Les LimThis should be good.
Comment #26
tim.plunkettTagging. Didn't review.
Comment #28
disasm CreditAttribution: disasm commented#16: 1938318-convert-book_remove_form-routing-16.patch queued for re-testing.
Comment #29
Crell CreditAttribution: Crell commentedThe bot still disagrees. :-(
Comment #30
Les LimFor 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.
Comment #31
Les LimComment #32
Crell CreditAttribution: Crell commentedSince 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. :-)
Er, wait a sec. The YAML comes back as "TRUE" or "FALSE", not as booleans? That smells funny to me...
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?
Comment #33
Les LimAnd 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.
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).
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.
Comment #34
Crell CreditAttribution: Crell commentedHm. 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?
Comment #35
tim.plunkettNow that we have #1938600: Add a FormInterface replacement for confirm_form(), BookRemoveForm should extend ConfirmFormBase.
Comment #36
Crell CreditAttribution: Crell commentedComment #37
disasm CreditAttribution: disasm commentedThis is a Symfony limitation on requirements. This method comes from Route.php:
Comment #38
tim.plunkett#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.
Comment #39
disasm CreditAttribution: disasm commented#30: 1938318-convert-book_remove_form-routing-30.patch queued for re-testing.
Comment #40
ParisLiakos CreditAttribution: ParisLiakos commentedrestoring status
Comment #41
disasm CreditAttribution: disasm commentedThis now uses services YAML syntax.
Comment #42
ParisLiakos CreditAttribution: ParisLiakos commentedBesides 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
Comment #43
tim.plunkettDid this have a visible menu entry or breadcrumbs before? If so, this is still needed.
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.
{@inheritdoc}
Is this actually needed? We should find a way to just use booleans
Missing blank line and @file block
Missing docblock
missing @var
missing $container
@param \Drupal\...
Remove this
Set $this->node instead
Comment #44
kim.pepperRefactored to use confirm form, replaced with new NodeInterface, and fixed comments as per #43.
Comment #46
kim.pepperOK.
NodeInterface
may have been a bit ambitious. I also noticed a few other issues such asbuildForm()
not returning parentbuildForm()
and a t function aroundconfirmText()
.Comment #48
kim.pepperThis 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()
.Comment #49
tim.plunkettTesting something out. If this passes, we have another bug.
Comment #51
kim.pepperEnsure the node is in the request attributes before calling
_book_node_is_removable()
Comment #53
jibran#51: 1938318-book-remove-form-interface-51.patch queued for re-testing.
Comment #55
jibranI am working on this.
I am going to move
_book_node_is_removable
toDrupal\book\BookManager
.I am going to create
Drupal\book\BookManager::deleteBook($nid)
to remove.I think
node.services.yml
changes are not required becausecan be replaced with
_entity_access: node.view
Comment #56
dawehnerThis options have been connected with && before, so you have to set this on the route definition, _access_checks: 'ALL' is the option you need.
Yes that is right, there is no need for that, just use entity_access.
You should return static::DENY here probably instead of return FALSE;
It is a node, so let's type hint it more specific
Menu link delete should be replaced by the menu link storage controller ...
You have the full node available so maybe using $node->uri() is the way to to it
This would 100% conflict with the NodeAccessCheck which is currently in core (which cares about permission).
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?
Comment #57
jibranImplemented #55 and fixed #56. Thanks @dawehner for the great review.
Comment #59
jibranSome silly fixes.
Comment #61
jibran#59: 1938318-59.patch queued for re-testing.
Comment #63
jibran_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.Comment #64
jibran:/
Comment #65
tim.plunkettThis is blocked by #1947880: Replace node_access() by $entity->access()
Comment #66
tim.plunkettThis needs a reroll for FormBase, and is still blocked.
Comment #67
disasm CreditAttribution: disasm commentedStarted with a straight reroll, and then updated BookRemoveForm for changes in FormBase. Still blocked on #1947880: Replace node_access() by $entity->access().
Comment #69
jibranCan you add a pass patch as well?
Comment #70
xjmThanks 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.
Comment #70.0
xjmUpdated issue summary.
Comment #71
tim.plunkettThis is the second-to-last confirm_form. No interdiff because its just been too long.
Comment #73
tim.plunkettmenu_link_delete() is more complex still, we need to continue to use that.
Comment #74
dawehnerWow, good catch!!
Comment #75
tim.plunkettComment #76
webchickNot sure why this is major, since it's not blocking a major. Nevertheless..
Committed and pushed to 8.x. Thanks!
Comment #78
BerdirSorry 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...
Comment #79
BerdirAh whatever, I think this only affects some weird 5.3.x versions, which we no longer support, so let's just forget about it...