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.
Issue: #1606794: Implement new routing system
Change notice: http://drupal.org/node/1800686
Comment | File | Size | Author |
---|---|---|---|
#120 | drupal-1800998-120.patch | 64.33 KB | dawehner |
#120 | interdiff.txt | 1.92 KB | dawehner |
#111 | drupal-1800998-111.patch | 64.08 KB | dawehner |
#111 | interdiff.txt | 3.03 KB | dawehner |
#109 | drupal-1800998-109.patch | 64.16 KB | dawehner |
Comments
Comment #1
xjmComment #2
dawehnerBefore someone is working on that, i do think we should wait until it's clear which route we are going in core.
Comment #3
xjmRe-postponing. :)
Comment #4
dawehnerAssign to myself to figure out how far we can get at the moment.
Comment #5
dawehnerhey
Comment #6
dawehnerThis patch allows you to use a view with the router and even all kind of arguments.
This does not remove any big chunks of code yet, due to sanity to not remember specific support for specific stuff.
Comment #7
dawehnerjust posting the newest patch.
Comment #8
dawehnerboom
Comment #10
dawehnerLet's see how many tests fail with that version.
Comment #12
dawehnerRemoved rebuilding of the router, so let's see how much fails with that.
Comment #13
dawehnerJust runned the first view via a subrequest and it worked fine!
Comment #14
dawehnerWrong patch.
Comment #16
dawehnerA couple of workarounds later.
Comment #18
dawehnerLet's see how many test failures are left, this is feeling better.
Comment #20
dawehnerLet's upload the first green patch.
Comment #22
dawehnerMaybe that one.
Comment #24
dawehnerHe, failed on an actual new test added by that patch :)
Comment #25
damiankloip CreditAttribution: damiankloip commentedDo you think we should give this a new name? like uses_route or something?
Seems like the logic from collectRoutes (when there is some) should be on the RouteSubscriber instead?
Don't need a blank line after this.
So I guess we keep this for tabs etc...? So maybe at some point we can change this to executeMenuLocalTasks or something.
Could we just replace this with
$this->enableModules(array('system'))
? Then it should install those schemas anyway I think.Instead of doing this, couldn't we just store the views args in a views_args array in attributes? Not sure.
Comment #26
dawehnerWell, we need one which fits for both hook_menu and uses_route, as you will for now require both (but not for feed).
I'm wondering whether we should do all of that in this patch or clean it up later.
Seems like the logic from collectRoutes (when there is some) should be on the RouteSubscriber instead?
I'm not sure, as especially the display plugin have to be able to define the routes, as the logic is on there, and probably should stay there.
I agree that potentially this collectRoutes on the ViewExecutable should be skipped and the display would be used directly.
We have to, as menu links is pretty important :) We have to maybe add executeMenuLocalTasks and keep executeHookMenu for the normal links, but that's all not sure yet.
So enable system would cause quite a lot of new modules, so I would really suggest to stay away from that, as three items is way faster.
Well, as far as I know you can only map url values to attributes in the request, but not to a subarray.
Comment #28
dawehner#26: drupal-1800998-26.patch queued for re-testing.
Comment #30
dawehnerUps.
Comment #31
Crell CreditAttribution: Crell commentedI'm assuming this can get refactored to an object later?
This is usually a sign of a design flaw. Why not make a RoutableViewInterface with a collectRoutes() method. Then a display plugin can implement that interface and method or not. If not, then it has no need for this method at all.
The subscriber above can then simply wrap an instanceof check around the method call, which would avoid having or calling null methods like this.
Why is this necessary? This should be handled by the view subscribers later on. This is on a Page display, which, assuming that survives B&L, should be responsible only for the body area of the page.
I think we may want to consider a pluggable method similar to access control for extended validation. The _requirements array works for simple values (regexes and single-key values), but we may need more complex rules. Or maybe we can get away with just setting _requirements like we do for access, and having extended validators? They'd return a 404 instead of a 403 the way access does.
That may be best as a spinoff issue if we can justify it as not a "new feature".
I think this should be _content, not _controller, so that the normal Drupal controller can take over. Assuming we're dealing with a page display, not a feed, which should be handled differently. (A Page display uses the Drupal controller, whatever it is; a feed has its own controller, since it is responsible for the entire response.)
Oh this is not good at all. We are trying to eliminate functions as access callbacks since they cannot be lazy loaded.
Since Views access control is a plugin, it should be able to simply have a _my_plugin: TRUE flag, like the existing access checkers, and then we call it like any other access checker. That would be much cleaner than bridging to functions.
I think the preferred way to structure this sort of check is:
We should be able to do this natively in the router now. No need to do so here.
I would love to do something cleaner than this. Don't arguments have to have a machine name in the View config? Can't we use those? Or am I confusing them with normal filters?
See above for why I think this checker is a bad idea. Much of the work that's gone into Drupal 8 is exactly so that modules like Views don't need to build their own systems on top of core that core should already be doing. It's doing it now, so let's use it directly. :-)
Ack. No it's not. Why do you need this table check here? Avoiding rebuilding before the router is initialized shouldn't be your problem. If the builder needs that check internally we can add it, although I suspect that's a sign of some other problem.
Comment #32
dawehnerAh someone providing feedback that's good, thanks!
Let's talk about the access part in IRC, but here a bit of explanation why I went this route:
Currently a view can be embedded into a page/block/where-else and all of these places use the same kind of access check via the access plugins. If you would just use access subscribers, it seems to be impossible to use access on other levels like blocks?
I guess on the longrun we could use access checks on subroutes and just provide configuration via the views UI?
Good idea: Here is a follow up for that: #1918860: Move views_get_applicable_views into a class
Yeah I'm not yet into interfaces that much. In general I'm wondering whether we should keep our "uses_route" on the plugin definition, as it's exactly used for that: Determine which plugins have which feature. Do you agree with dropping the uses_route key, and allow to fetch views_get_applicable_view somehow the information from interfaces as well. Do you have an oppinion whether RouteDisplayInterface makes more sense, as it's here we talk about displays, not about view/viewExecutables.
At the moment lower levels, then the displays have to be able to change the response. For example the rss style, wants to set it to application/rss+xml, or some kind of handler might want to set the response code to any specified value. (template_preprocess_views_view_rss() for reference, even this is certainly not the best place to do it).
Yeah all more like pager manager related stuff need that. Should I create a WSCCI issue for that?
See my reason for that above, but I'm 100% open for cleaner solutions.
Can you specify \Drupal\foo\Bar\Baz::foo() and it will be loaded if available, when you call cuf(a)?
Just to summarize: Every access plugin should provide it's own route access subscriber (if needed) instead of using callbacks? This seems to be a great idea for me, will refactor it later.
I tried that before, but
$variable_names = $route->compile()->getVariables();
seems to be empty because the view ID is not part of the URL.They absolutely do.
Additional I have been thinking of the place of argument default plugins in this new world, but again, I got stopped because they are still not only part of the route.
Some of the tests complained about that, but I didn't went into more research: If this cache clear would be part of the container, we could inject the router builder, so we would be sure that the table exists all the time, as the builder knows about it?
Comment #33
dawehnerAnd the interdiff, just for reference.
Comment #34
Crell CreditAttribution: Crell commentedRe interfaces: The one drawback of an interface as the "flag" is that it's only useful at runtime. You have to have the class parsed into memory at least in order to do class_implements(), or have an instantiated object to do instanceof. So for the code I highlighted above an interface is all you need. If you need a "get me all plugin types that will use routes" operation, that can be done with interfaces but gets very expensive.
(That said, if we can do static code analysis, like the registry used to and the annotation support does now, that could be a good way around it. That would be awesome but is not something we can do in this issue. So, maybe do both for now and see if we can rip out the flag later.)
I am not convinced that it's wise to allow any handler/style to change things like the mime type. If you're returning RSS, that's something the display plugin knows and should control, and that's the end of it.
call_user_func_array('\My\Class\Here', 'aMethod'); will call aMethod statically on \My\Class\Here. That's only marginally better than a function. call_user_func_array($an_object, 'aMethod'); will call aMethod on that object. The Routing system has two alternate syntaxes inherited from Symfony that the ControllerResolver takes care of: \My\Class::aMethod instantiates \My\Class first and then calls aMethod, while some_service:aMethod retrieves some_service from the DIC and then calls aMethod. You'd have to reimplement that here if you wanted to use those, though.
The preferred approach is indeed an access checker service; the system is designed so you could write 30 of them, but only the ones that care about a given route will be instantiated when needed. And "When needed" is up to the checker to decide based on anything it wants on the route definition.
The view is in the attributes array, and is therefore available to the ParamConverter system, which already handles any entity whose parameter name matches its entity machine name. If you need more, write a new ParamConverter.
Comment #35
dawehnerAt the moment this is not really the case. Let's take the RSS example, as it's a good one. The display would be not an RSS display but an abstract feed display. This allows you to write for example an atom style plugin without having to worry about the display, even you could still set the http headers. Happy enough you actually wrote that one.
Another example of that, which is indeed already in core is the REST_Export display that allows any kind of output. Sure you could ask the style/row/whatever plugins for explicit mime types changes as well, but who can know what a potential contrib module want to change.
Comment #36
Crell CreditAttribution: Crell commentedSummary from IRC conversation with @dawehner:
1) Between the access checker system, custom request listeners, and the fact that a controller can still throw an AccessDenied/NotFound exception if it wants, we're not going to add a dedicated validation system at this time. There's enough places already that you can do such things that we're going to punt on it. If it turns out we need a common system contrib can add it easily, and we can revisit moving it into core in Drupal 9.
2) Views access control will be 2-part: A Views Access Plugin will be responsible ONLY for annotating a route definition with appropriate data. Actually enforcing that access control will be handled by a normal Access Checker as it already exists today.
For instance, for the Role-based access control, the Views Access Plugin will only be responsible for adding requirements: _role: "do something cool" to the route definition. A completely Views-agnostic (even if it may just happen to live in the Views module) Access Checker will then enforce that, exactly as the PermissionAccessChecker does now. Then you can specify _role yourself in a route definition, or Views can, or some other module can, or whatever. Actual enforcement is entirely decoupled, which is good.
3) Non-Page Displays (eg, Feeds) will be wired to the route directly as a _controller. There's no need for an extra layer there. Style plugins may know what mime type their return data is supposed to be, but will *not* have a Response object themselves. That is, the Feed display will call getContent() on the Style plugin, which will return a string, and getMimeType() on the Style plugin, which returns another string. (Or something along those lines.) The Feed will then use that data to assemble a Response object. The Style plugin and lower will have no dependency on HttpFoundation or the concept of HTTP.
Comment #37
dawehnerjust tracking work.
Comment #38
dawehnerAdded a full test for role based accessChecker.
Comment #40
dawehnerThis could fix a tone of the problems
Comment #42
dawehnerLet's see.
Comment #44
dawehnerDear Drupal Kernel, please just register by test bundles!
Comment #46
dawehner#44: drupal-1800998-44.patch queued for re-testing.
Comment #48
dawehnerLet's get rid of the installation problem and see which tests are still broken.
Comment #50
dawehner#48: drupal-1800998-48.patch queued for re-testing.
Comment #52
dawehnerWow, days of debugging for such a trivial change.
This patch might include more changes, but I just post the important difference.
Comment #53
Crell CreditAttribution: Crell commentedThis should probably switch over to Drupal:: now that we have that.
As of last night, page callback and access callback can be left out entirely. Just specify "route" and the machine name of the route and the rest will be handled automatically.
Ibid.
I don't get why we have this, when it's just a permission check. Why not just use the existing permission checker?
I know this is in a test module, but... huh?
Also, we now have _content and _form default keys in routes. I wonder... should we add a _views key that we can then enhance to the appropriate View controller, just like we do with pages and forms? Rather than hard-coding the controller into the route definition? I'm not sure if that makes sense here, but it's worth asking.
Comment #55
dawehnerI see your point, as we have multiple router items using the same controller. One problem is thought that it feels like a hack.
_form and _content are used both for wrappers, though a view controller does seems to be anything that is needed already, or not? Maybe we should think about something like that on a follow up?
Well, yeah I struggled with that, but I also didn't wanted to remove test coverage for that usecase. There should be certainly the test which uses $view->access() directly, but I see that it's quite complicated for just a "simple" test.
Comment #57
dawehnerBlocked on #1938960: _menu_translate() doesn't care about new routes currently.
Comment #58
dawehner#55: drupal-1800998-54.patch queued for re-testing.
Comment #60
dawehnerRerolled the patch.
Comment #61
dawehnerForgot some files to add.
Comment #62
dawehnerAdd tag.
Comment #64
dawehner#61: drupal-1800998-61.patch queued for re-testing.
Comment #66
dawehnerFixed one issue in the node rss test, which tests for rss.xml/foo/bar, which simply returns 403 in the world of route items.
The current problem is that the AccessTest for the dynamic access plugin doesn't even consider to execute the DynamicTestAccessCheck when called with the "test_access_dynamic/foo/bar" path, @see AccessTest::testDynamicAccessPlugin() and DynamicTestAccessCheck::access().
This access method though is called for test_access_dynamic, that's why i'm really confused.
Comment #68
dawehnerComment #69
tim.plunkettI always get confused when I see this in my list :)
Comment #70
Crell CreditAttribution: Crell commentedRoles are not defined by hook_permission. They're defined by a user.
We should probably make this _role, not _role_id. We're using _permission, even though what's being specified is the permission machine name rather than the human-readable name.
Hm. I don't believe any other access checkers do this comma-separated list thing. I don't know if that's good or bad. :-)
I suppose since Views does allow "OR" for role and permission, it is necessary. In that case, we should probably do the same for _permission checks to be consistent.
Because then we can more easily mirror this check (which I assume has to be here for non-route views?) like so:
_permission: "something, access all views"
This @todo can now go away in favor of adding a route_name. Though I don't fully understand what this code is doing as it looks like a duplicate of the previous block in the patch, which says it's the same method, so I'm totally confused. :-)
I have no idea why you're putting a _ prefix on this method...
But all methods should have a visibility declared.
The idea that we can do this rather than drupalGet() makes me warm and fuzzy inside. :-)
This should be Drupal::getService('router.builder') now.
What I don't get about #66 is why DynamicAccesCheck is here. It appears to be just for the test module, so... what's it testing? If that's a problem, why not eliminate the problem? I still don't get it. :-)
The role access checker could arguably be spun off to its own issue if you want, to keep the patch size down.
The breadcrumb tests are worth retesting in light of recent patches that went in just in the last day or so from Tim. Will retest in a moment. I don't know why testbot is not showing me what the actual failures were on the other tests...
Comment #71
Crell CreditAttribution: Crell commentedComment #72
dawehnerThanks for another great review! Sorry for sticking some debugging changes in there.
Yeah the plan was always to split up the role changes, so here we go: #1956896: Add role based access checker
Btw. you convinced me to remove the DynamicAccessTest as it was mostly about checking some code in hook_menu which is removed with that patch, so this would just test whether the route system would be able to support dynamic access arguments, which for sure can be done, if you use your own access checker.
Tim wanted to look at the breadcrumb tests, which is great!
I will fix your points once the role issue is in?
Comment #73
tim.plunkett#66: drupal-1800998-66.patch queued for re-testing.
Comment #75
moshe weitzman CreditAttribution: moshe weitzman commentedThis was mentioned during our REST call. We think this would help us keep Views' serialization working as we transition to HAL from JSON-LD. Thanks!
Comment #76
moshe weitzman CreditAttribution: moshe weitzman commented#1956896: Add role based access checker was just committed. Shall we get this moving again?
Comment #77
dawehnerYes, i will care about this on the weekend.
Comment #78
dawehnerYeah exactly. Well on the longrun you might be able to just replace everything by subrequests. Just for example an embedded view in some kind of template
is simply just that.
That's why there is #1984528: Follow-up: Allow for more robust access checking
Oh right, that's what we alread added in the patch.
That's just my own way to make tests run faster, i'm sorry.
I also removed all this dynamic test bits.
I know the breadcrumb tests still fail, but this seems to be just a small detail.
Comment #80
dawehnerThe javascript failure seemed to be just random.
The failure in the breadcrumb bit is caused by the fact that even the menu_link table has the route_name, it's not stored when you add a new menu_link via the UI/API, unless you specify the route_name.
This patch adds a small helper function which fetches the route_name from the menu_route table and stores it on the menu_link.
Comment #81
Crell CreditAttribution: Crell commentedThe menu_router table is hopefully going away. What exactly are you trying to do here? Remember that path does not map 1:1 to route anymore.
Hitting the DB directly is almost always wrong at this point. :-) (Even then, you should be injecting the connection.)
This is still wrong, I think. Although what right is depends on what happens with Scotch. :-/
Comment #82
dawehnerThanks for the review!
There is another issue to allow inject dependencies into entity controllers: #1909418: Allow Entity Controllers to have their dependencies injected
Can we agree to get this in like this, as it seems to be the way to do it at the moment (without scotch).
Yeah you are totally right. We can ask the route system to give us the matching route.
Comment #84
Crell CreditAttribution: Crell commentedYeah, I'm not sure what the right way is there, so until then "it works" is good enough, I suppose.
Comment #85
dawehnerlet's also catch some exceptions so the installation works again.
Comment #87
dawehner#85: drupal-1800998-85.patch queued for re-testing.
Comment #89
tim.plunkettEvery single upgrade path test is hitting a 30 second max execution time.
No idea why :(
Comment #90
Crell CreditAttribution: Crell commentedI have seen cases before where an infinite loop that involves a menu rebulid will hit the 30 second timeout before hitting the 100 function stack limit. That makes it look like a timeout when it's really an infinite loop problem. Perhaps that's the case here?
Comment #91
BerdirYes, the 100 function recursion limit is a xdebug feature, without that exception, a recursion goes on until it hits the max execution time.
Comment #92
dawehnerYeah there is a recursion involved.
Comment #93
ParisLiakos CreditAttribution: ParisLiakos commentedAdditionally? Also i think this should be rephrased to:
Additionaly to implementing the interface "uses_routes" should be used into
the plugin definition.
i guess # is not needed
inheritdoc
this took me a while to figure out..even if there is a @todo, maybe transfer it to a seperate protected method?
is this still a case?
Comment #94
Crell CreditAttribution: Crell commentedTry: "In addition to implementing the interface, specify 'uses_routes' in the plugin definition."
Comment #95
damiankloip CreditAttribution: damiankloip commented'.. a URL'
If we are checking this here, we should change the method signature to ($account = NULL)
This is an associative array of
rid => rid
now, so do we need to do this? array_intersect would still work fine.hmm, good question. I think I get bad vibes about doing that - not sure though. Talk about it in Portland? :)
If a user didn't have a permission wouldn't this ternary evaluate to FALSE, so NULL would be used? So instead of failing access we are passing it on?
Comment #96
dawehnerYes, but maybe that's another issue of the bootstrap like on the other issue.
The only place which calls this code is in DisplayPluginBase::access() which passes in the account, so there is no need to check.
Good point.
No it should be NULL, because FALSE means in the new access manager system that the user will not have access at all to the route. NULL means (let's other plugins decide, so for example the role based one can tell it's opinion).
Comment #97
Crell CreditAttribution: Crell commented"a URL", not "an". :-)
Other than that... I think we're finally done here.
Comment #98
dawehnerI have to trust you :) The relation between "an URL" and "a URL" in core is 43:2.
Comment #99
dawehnerThis time with the actual changed string.
Comment #100
Crell CreditAttribution: Crell commentedComment #100 FTW!
Comment #101
swentel CreditAttribution: swentel commentedExtreme nitpick .. :)
Comment #102
xjmWe found a pretty serious bug testing this patch (actually several, one in the patch and one outside it). I'll re-confirm and then post details.
Comment #103
xjmYeah, unfortunately, this is majorly broken. At least some contextual filters do not work with this patch.
STR:
/node
frontpage view to have the pathfront
(to avoid a route conflict withnode/{node}
in the node module; do we want to consider changing the path for/node
? Followup?)./front
. You should properly get a 404./front/page
. In HEAD, you will correctly see your page node. With this patch applied, you get a 404.Also, this issue feels like at least a major to me. Could we even ship without doing this or severely compromising WSCCI's goals?
Note that I also got a similar result with roles, but roles are also broken in HEAD, which I'll file separately and link here.
Comment #104
xjm#1995868: Fatal when using a role contextual filter is the other issue we discovered in testing.
Comment #105
dawehnerThanks for testing this patch!
Changed a couple of things here:
Comment #107
tim.plunkett#105: drupal-1800998-105.patch queued for re-testing.
Comment #109
dawehnerWhile fixing the remaining bugs I realized that: #1998182: Glossary view is broken
Comment #111
dawehnerComment #112
dawehnerRemoves the tag.
Comment #114
dawehner#111: drupal-1800998-111.patch queued for re-testing.
Comment #115
R.Hendel CreditAttribution: R.Hendel commentedI have tested following cases and found no errors.
Node-views:
Additional info: multiple arguments do not work completely with "," or "+" as conjunctors, but this seems to be another issue for me. Both working well using "all" as argument.
Additional info: Works only using "all" as argument.
User-based views:
Taxonomy-views:
pagers:
In my opionion the patch works fine and can be committed.
Comment #116
dawehnerThank you very much!
Comment #117
tim.plunkettThanks for manually testing this.
The 20K of added tests in #105 is awesome! @dawehner++
Comment #118
katbailey CreditAttribution: katbailey commentedAlex asked me to look at this. Certainly from a routing perspective I don't have anything to add as Crell has already given it the thumbs up and it looks pretty awesome to me. The only thing that jumped out at me was the
Drupal::service()
call in MenuLinkStorageController but I see there is already a separate issue to figure that general problem out over in #1909418: Allow Entity Controllers to have their dependencies injected.So, +1 to RTBC :-)
Comment #119
alexpottRunning the tests locally I get a fail...
Also we need to open follow up for...
Comment #120
dawehnerFor some odd reasons views_preprocess_html() now calls user_access, so the test needs database table now.
Comment #121
dawehner#2003864: Router rebuild is triggered before router table created.
#2003866: Invent argument validation in the router entries
Comment #123
dawehner#120: drupal-1800998-120.patch queued for re-testing.
Comment #124
tim.plunkettRe-RTBC, the minor changes since #117 look great.
Comment #125
alexpottCommitted 7002e6b and pushed to 8.x. Thanks!
Comment #126
YesCT CreditAttribution: YesCT commentedadding tag
Comment #127
ParisLiakos CreditAttribution: ParisLiakos commentedi am not sure whether we need a notification here? it is a straight conversion
Comment #128
YesCT CreditAttribution: YesCT commentedyeah, other conversions are not getting individual change records.
this is part of #1971384: [META] Convert page callbacks to controllers
Let's just include this is the issues list. of https://drupal.org/node/1800686
Comment #129
ParisLiakos CreditAttribution: ParisLiakos commentedrestoring title
Comment #130.0
(not verified) CreditAttribution: commentedUpdated issue summary.