Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Convert this page callback to a new-style implementation, using the instructions on http://drupal.org/node/1800686
Comment | File | Size | Author |
---|---|---|---|
#59 | book-1945390-59.patch | 16.87 KB | dawehner |
#54 | interdiff.txt | 7.11 KB | tim.plunkett |
#54 | book-1945390-54.patch | 16.69 KB | tim.plunkett |
#50 | drupal8.book-module.1945390-50.patch | 18.48 KB | disasm |
#50 | interdiff.txt | 2.18 KB | disasm |
Comments
Comment #1
Alan Evans CreditAttribution: Alan Evans commentedLooking into this one...
Comment #2
Alan Evans CreditAttribution: Alan Evans commentedComment #3
Alan Evans CreditAttribution: Alan Evans commentedFirst 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.
Comment #4
Alan Evans CreditAttribution: Alan Evans commentedComment #5
Crell CreditAttribution: Crell commentedUnfortunately, 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. :-)
The cache is now injectable, I think, so let's inject it.
This should be protected. Drupal standards say to never use private. Also, lowerCamel name.
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.
Method names should be lowerCamel. So $this->adminTableTree().
Method names should be lowerCamel, and protected, not private.
Comment #6
Alan Evans CreditAttribution: Alan Evans commentedThanks for the brilliant review, @Crell. Minor changes done, looking into the injection changes (cache, user_access and node_access) ...
Comment #7
Alan Evans CreditAttribution: Alan Evans commentedProgress patch:
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 ...
Comment #8
Alan Evans CreditAttribution: Alan Evans commentedComment #9
Crell CreditAttribution: Crell commentedIgnore node_access() and user_access() for the moment. Those should get their own issues.
Comment #10
Alan Evans CreditAttribution: Alan Evans commentedWell, looks like I totally ballsed the last patch up, but other than that I'm sure it's fine ...
(investigating ...)
Comment #11
Alan Evans CreditAttribution: Alan Evans commentedNo idea, all green locally - test logs look weird (lots of file writing errors) ... rerolling against latest 8.x showed no differences. Requeueing for now.
Comment #12
Alan Evans CreditAttribution: Alan Evans commented#7: drupal-book-admin-edit-1945390-7.patch queued for re-testing.
Comment #13
Alan Evans CreditAttribution: Alan Evans commentedThose minor comment corrections ...
Comment #14
Crell CreditAttribution: Crell commentedWe haven't been doing much of that, but classy. :-)
This @see is now out of date.
I think that's the only thing left I could find.
Comment #15
Alan Evans CreditAttribution: Alan Evans commentedGreat, 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.
Comment #16
Alan Evans CreditAttribution: Alan Evans commentedComment #17
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 #18
Alan Evans CreditAttribution: Alan Evans commentedThanks - on it.
Comment #19
Alan Evans CreditAttribution: Alan Evans commentedComment #20
Alan Evans CreditAttribution: Alan Evans commentedwhitespace correction
Comment #21
Alan Evans CreditAttribution: Alan Evans commentedFollowing 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!
Comment #22
Alan Evans CreditAttribution: Alan Evans commented... 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.
Comment #23
tim.plunkettJust 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.
Comment #24
Alan Evans CreditAttribution: Alan Evans commentedI 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
Comment #25
ParisLiakos CreditAttribution: ParisLiakos commentedexceeds 80 char limit
AccessCheckInterface needs the full namespaced path..but thank go we can now use inheritdocs
#1906474: [policy adopted] Stop documenting methods with "Overrides …"
Comment #26
ParisLiakos CreditAttribution: ParisLiakos commentedComment #27
Alan Evans CreditAttribution: Alan Evans commentedRerolling and integrating these ...
Comment #28
Alan Evans CreditAttribution: Alan Evans commentedComment #29
Alan Evans CreditAttribution: Alan Evans commented(in the most recent patch also removed one redundant "Use" which, ironically, was not used).
Comment #30
ParisLiakos CreditAttribution: ParisLiakos commentedlooking really good! nice job:)
I guess we should make those inheritdocs too, sorry forgot to mention it above
And maybe since you are there move this inline comment above the statement:)
Otherwise this looks good to go if bot agrees
Comment #31
Alan Evans CreditAttribution: Alan Evans commentedThanks 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.
Comment #32
Alan Evans CreditAttribution: Alan Evans commentedWOOHOOOOO I got interdiff file number 5000!! Is there a prize?
Comment #33
ParisLiakos CreditAttribution: ParisLiakos commentedhehehe:) nice
The thing with inheritdoc, is that it has to be alone to work
So while this works:
This won't:
It wont be parsed from api module. So, lets remove those extra lines, i dont see anything worth saving
Comment #34
ParisLiakos CreditAttribution: ParisLiakos commentedComment #35
kim.pepperAlan Evans, are you still working on this?
Comment #36
Alan Evans CreditAttribution: Alan Evans commentedSorry, 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..
Comment #37
Alan Evans CreditAttribution: Alan Evans commentedRebase 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 ...
Comment #38
Alan Evans CreditAttribution: Alan Evans commentedBook 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.
Comment #39
Alan Evans CreditAttribution: Alan Evans commentedComment #40
Alan Evans CreditAttribution: Alan Evans commentedAnother 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!
Comment #41
dawehnerThis is really good so far!
If you just have a MENU_CALLBACK you can drop it entirely.
Should be "Contains \"...
You want to return NULL if your two checks don't apply. Can't you also call $node->access('view') directly?
Instead of calling entity load you could inject the entity manager and from there get the storage controller for the menu_link entity type.
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.
Comment #42
dawehnerWe could get rid of the access checker entirely if #1987768: Convert node_page_view() to a new style controller is in.
Comment #43
disasm CreditAttribution: disasm commentedreroll plus changes in #41, plus switched to using FormBase.
Comment #44
disasm CreditAttribution: disasm commentedComment #45
tim.plunkettIn 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
Comment #46
disasm CreditAttribution: disasm commentedComment #47
tim.plunkettNot sure why NodeStorageController doesn't have an interface, but MenuLinkStorageControllerInterface exists.
Comment #48
disasm CreditAttribution: disasm commentedUsing MenuLinkStorageControllerInterface in AdminEditForm
Comment #49
dawehnerJust put a _title property unto the route defaults and you can remove the hook_menu entry as it is.
Let's replace that with current_user in the future.
This one should also use $this->t()
Do you think we can typehint on NodeInterface instead?
Comment #50
disasm CreditAttribution: disasm commentedreroll 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.
Comment #51
jibranCan we convert this to OO.
Comment #52
disasm CreditAttribution: disasm commentedLets 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.
Comment #53
jibranOk then it is ready to fly.
Comment #54
tim.plunkettI 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?
Comment #55
disasm CreditAttribution: disasm commentedThis 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.
Comment #56
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 #57
jibran#54: book-1945390-54.patch queued for re-testing.
Comment #59
dawehnerHave we figured out any kind of standard for this optional parameters?
This is just a rerole.
Comment #60
jibranBack to RTBC. I am ignoring #55. We can do it in follow up issue.
Comment #61
jibranComment #62
webchickCommitted and pushed to 8.x. Thanks!
Comment #63.0
(not verified) CreditAttribution: commentedChange records don't link.