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.
For instructions on how to convert a page callback into a controller, see the WSCCI Conversion Guide.
Part of #1971384: [META] Convert page callbacks to controllers
Comment | File | Size | Author |
---|---|---|---|
#61 | 1974376-aggregator-categories-controller-61.patch | 11.4 KB | stella |
#61 | 1974376-55-61-interdiff.txt | 1.01 KB | stella |
#60 | 1974376-aggregator-categories-controller-60.patch | 9.78 KB | stella |
#60 | 1974376-55-60-interdiff.txt | 6.71 KB | stella |
#55 | 1974376-aggregator-categories-controller-55.patch | 11.38 KB | CB |
Comments
Comment #1
kim.pepperAs discussed with @berdir in IRC, creating an implementation of AccessCheckInterface to check whether any categories exists seems like overkill for this conversion.
As a result, the 'Categories' menu item now appears, even if there are no categories.
Comment #2
kim.pepperComment #3
ParisLiakos CreditAttribution: ParisLiakos commentedthis should not be db_query bit $this->database->query. You need to inhect it,like the entity manager
same with this. you need to inject config.factory service
Thats bad. I would like to convert it to an AccessCheckInterface cause when #15266: Replace aggregator category system with taxonomy goes in, it will be possible for anyone to just delete the category field..we need a way to tell if we should show a menu link or not
Comment #4
kim.pepperInjecting all the dependencies.
Still to create an AccessCheck
Comment #5
kim.pepperNow with accesscheck implementation.
Comment #6
dawehnerIt seems to return a build array instead of a render list.
Comment #7
kim.pepperOops! That was copied and pasted from the original function.
Updated comment.
Comment #8
BerdirI think you can combine multiple access tags and don't have to worry about the permission in here but just the query.
I hoped we could get rid of this check as you can also just disable the menu link if you don't want it. But as this is straight conversion, let's keep it for now. Maybe we can do that when that thing is a view, then you can just disable the view...
Comment #9
dawehnerLet's make these two comments consistent.
Needs docs.
I'm not sure whether put helpful helper methods always into the controller is the best thing you can do, because it removes reusability. Additional the code style standard is "@todo" see http://drupal.org/node/1354
Comment #10
kim.pepperThanks for the review @dawehner. This fixes issues in #9.
Comment #11
kim.pepperOops! Sorry @berdir. Missed your comments. The following moves the
access aggregator feeds
check back to the router.Comment #12
BerdirI think that would need to be _permission, no?
Also, I noticed, again, that this currently doesn't work.
I don't understand why PermissionAccessCheck is implemented as it is, that just makes no sense to me.
Cases like this seems to be the 80% use case, where you have multiple conditions and each must be true. Right now, the permission access check only grants and doesn't deny access. (Access is only implicitly denied if nobody else gives access explicitly).
Why would you ever want that?
Comment #13
BerdirLet's see what Crell has to say about that when he's back from holiday.
Comment #14
BerdirI know this was modelled after hook_node_access() but that seems like a completely different situation to me.
For hook_node_access(), the caller doesn't specify what kind of checks he wants to have applied, he just asks, "Has anyone an opinion on whether this node should be accessible to the current user?". There it's obvious that an implementation should only explicitly grant or deny if it has something to say.
However, for a route, the "caller"/definition explicitly specifies what kind of checks he wants to have applied. He defines which implementations he wants to invoke with which configuration. Why would such a definition as above ever be an "grant access if either that or the other thing is true?" :)
Note: I'm not saying to change the access handling logic. I'm just suggesting that implementations like _permission or the entity access stuff should return either TRUE or FALSE and not NULL.
Ok, enough ranting :)
Comment #15
kim.pepper@Berdir makes total sense. I'd like to hears @Crell's point of view on this.
Comment #16
dawehnerAs far as I understand the access checker it's a system where the access check manager asks: Who can tell us something about the access to a route?
Can you 100% say the user should have access, or can you say the user should not have access, or you can't answer the question.
As far as I understand is that there is a need for OR based access. For example on a view, you want to have a permission called "access all views",
but also a specific access like per role. In general it seems to be that you should be able to specify whether you want to connect the parts with AND/OR. Maybe this would solve the confusion?
Comment #17
andypostCurrently OR brings a lot of troubles - once we trying to convert csrf-token protected pages we stuck with #1798296: Integrate CSRF link token directly into routing system
It means that no way to get success from csrfAccess AND check other permissions
Comment #18
Berdir@dawehner: That's a nice counter example but that use case only works because you have two different kind of checks (permission + role). You can't do that for the admin permission + configured permission anyway for example.
Also "Who can tell us something about the access to a route?" is IMHO not true. As said above, it's true for node/entity access, but for routes, we have defined the access checked that should be done explicitly, those classes are just the implementation of our definition.
Shouldn't the whole views access logic be encapsulated in a views entity access controller anyway?
Comment #19
dawehnerWell, if you could just call $view->access() all the time, this statement would be certainly true. It's another case when you do have to make access
in a performant way. I really want to avoid the need to load the view object, all kind of plugins etc. to just determine the access. The route based access system seems to be a good way to handle this need.
Comment #20
dawehner@berdir
Just in case you are interested there is an issue to have an access controller: #1870758: Implement an entity access controller for a view
Comment #21
ParisLiakos CreditAttribution: ParisLiakos commentedcorrect status
Comment #22
kim.pepperI've moved 'access news feeds' back into the access controller so the are AND'ed and rerolled on latest head.
Comment #23
kim.pepperComment #25
Crell CreditAttribution: Crell commentedJust an FYI, I'm not ignoring this issue but the stalking bits seem related to: #1986640: Support AND/OR conjunctions for permission checks and #1984528: Follow-up: Allow for more robust access checking, so I've commented on those.
Comment #26
BerdirWorks for me, following those issues and releasing you from this one ;)
Comment #27
dawehnerShouldn't that be rather _content instead?
Comment #28
kim.pepperChanged to _content as per #27.
This also fixed an issue where the render array in adminOverview() wasn't being returned. I think this is what is making all the tests fail.
Comment #29
ParisLiakos CreditAttribution: ParisLiakos commentedunneeded newline
lets use \Drupal::moduleHandler
Comment #30
kim.pepperAdds fixes in #29
Comment #31
Crell CreditAttribution: Crell commentedWhy not just go ahead and inject the module_handler service? It's unclear to me if we can get away with that post-July 1 at this point, so let's do it while we're here.
Comment #32
kim.pepperInjecting all the things...
Comment #33
ParisLiakos CreditAttribution: ParisLiakos commentedthanks kim.pepper!
Comment #34
alexpottNo longer applies... needs a reroll...
Comment #35
kim.pepperRe-roll.
Comment #36
kim.pepperSetting back to RTBC (assuming green).
Comment #37
kim.pepperRe-roll.
Comment #39
kim.pepper#37: 1974376-aggregator-categories-controller-36.patch queued for re-testing.
Comment #40
kim.pepperRe-roll.
Comment #41
tim.plunkettLooks good to me, thanks for keeping it going @kim.pepper
Comment #42
kim.pepperRe-roll after #1974372: Convert aggregator_page_sources() to a Controller went in.
Comment #43
alexpottwe can replace url with the an injected url generator service.
Comment #44
kim.pepperInjected PathBasedGeneratorInterface for all url generation as per #43.
Comment #46
ParisLiakos CreditAttribution: ParisLiakos commented#44: 1974376-aggregator-categories-controller-44.patch queued for re-testing.
Comment #47
ParisLiakos CreditAttribution: ParisLiakos commentedwe might want to split the controller later, its __construct start getting big:)
Comment #48
alexpottNeeds a reroll as the AggregatorController has moved to core/modules/aggregator/lib/Drupal/aggregator/Controller :)
Comment #49
pwieck CreditAttribution: pwieck commentedRerolled.
Comment #50
CBRerolled against 8.x HEAD.
Comment #51
CBWhy do I always forget to change the status ... :|
Comment #52
CBUh oh! haha, hopefully one of us gets the green light. :D
Comment #53
pwieck CreditAttribution: pwieck commentedHa. Good practice for me. :-)
Comment #54
tim.plunkettIN BOTH:
Extra space after @var
IN #50:
Extra + sign
IN #49:
This is in Controller, not Routing.
----
One of you can reroll one of the patches :)
Comment #55
CBWerid about that extra plus sign - still ran fine for me.
Anyhoo, updated.
Comment #56
Gaelan CreditAttribution: Gaelan commentedRemoved tag.
Comment #58
dawehnerJust a tiny nitpick.
Needs one more empty line.
Comment #59
CBGah, will address these errors later when I have more time.
Comment #60
stella CreditAttribution: stella commentedPatch reroll to fix the errors
As for the nitpick item above, I couldn't find it in the file, so may have been fixed already.
Comment #61
stella CreditAttribution: stella commentedErrr forgot to include a file, so reroll
Comment #62
ParisLiakos CreditAttribution: ParisLiakos commentedthanks
Comment #63
alexpottCommitted 08f15f2 and pushed to 8.x. Thanks!
Comment #64.0
(not verified) CreditAttribution: commentedLink to meta task