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.
Updated: Comment 0
Problem/Motivation
Issues like #1938884: Replace the fallback user listing with a list controller clearly shows that with the new routing system views need a way to override existing route items
instead of just adding new items.
Proposed resolution
Do the magic required to be able to override an existing route.
This requires to replace the existing items and keep the route name!
The route rebuilding works the following way:
- Foreach static .routing.yml file a alter event is executed
- On this alter event views figures out that the route has to be overridden and replaces the route definition but keeps the route name
- Track in state that a certain view was used to replace an existing route
- After all static routing files are parsed the dynamic route event is fired
- Views now registers routes for each view which hasn't be used to replaced an existing route before
Remaining tasks
These TODOs are not required for now but are somehow important once we want people to use that in actual real world.
- Figure out how to do deal with slugs in the path: For example override taxonomy/term/{term} in the future #2110845: Allow overriding views to override paths with parameters
- Pull the raw values from the _raw_variables attribute.
- What should be done with the existing requirements, options etc. ? How do we merge things together
User interface changes
API changes
Related Issues
Original report by @dawehner
Comment | File | Size | Author |
---|---|---|---|
#61 | override-2027115-61.patch | 45.88 KB | dawehner |
#61 | interdiff.txt | 879 bytes | dawehner |
#59 | vdc-2027115-59.patch | 45.84 KB | tim.plunkett |
#59 | interdiff.txt | 4.34 KB | tim.plunkett |
#55 | vdc-2027115-55.patch | 45.53 KB | dawehner |
Comments
Comment #1
dawehnerThe idea is to check on the alter event whether it is needed to remove any existing items,
as we have added the routes by views already.
Comment #4
tim.plunkettRerolled without the change to the view yaml.
Comment #5
tim.plunkettComment #7
tim.plunkett666
Comment #9
tim.plunkettDrupal\views_ui\Tests\ViewListControllerTest::testBuildRowEntityList Argument 4 passed to Drupal\views\Plugin\views\display\PathPluginBase::__construct() must be an instance of Drupal\Core\Routing\RouteProviderInterface, none given /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/views/lib/Drupal/views/Plugin/views/display/PathPluginBase.php:37 /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/views_ui/tests/Drupal/views_ui/Tests/ViewListControllerTest.php:83
The PHPUnit test needs the route provider mocked
Comment #10
dawehnerEven if it kind of works now, the problem now is that you should not remove the existing remove, but replace that one with the one provided by views.
One reason is that otherwise the url generator ->generate method will fail at some point.
Comment #12
dawehnerJust tracking some work.
Comment #14
apaderno#12: vdc-2027115-12.patch queued for re-testing.
Comment #16
effulgentsia CreditAttribution: effulgentsia commented#1938884: Replace the fallback user listing with a list controller is postponed on this.
Comment #17
dawehnerHa, this works way better!
Comment #19
jibranthis should be entityQuery.
Can we add this namespace above or \ before namespace?
We can't mix @inheritdoc and description. Blank line is missing after the comment. @param description is missing.
@param description is missing.
doc block missing.
entity.manager now
Comment #20
dawehnerThank you for the review. Let's see how much passes now.
Comment #22
dawehnerSome bugfixes here and there.
Comment #23
dawehnerThis interdiff seems horrible.
Comment #24
damiankloip CreditAttribution: damiankloip commentedCan't we just add a user.local_actions.yml file instead?
Resets
If we unset, then the property isn't there anymore at all, even though it's a declared property. I think setting it to NULL.
This is all we can do for now, but we should add a todo to convert this? I'm hoping we can add some more stuff to the Views class for stuff like this.
Why the check for ViewExecutable here?
Needs a first doc line, I think the @return is wrong too.
full stop.
Generates
So alter routes just runs after collectRoutes has run and basically cleans up (removes) and routes that will be overridden by view?
Is the plan there to set that on the executable? Because that could be problematic if you need to use properties directly.
I guess this needs to be done still :)
Comment #25
dawehnerI had some issues while writing the patch, so this worked ...
Exactly, but yes, this is actually not the right approach. You would have to keep the existing route name to work as expected.
Not at all ;)
Comment #26
dawehnerMissing file.
Comment #28
tim.plunkettRerolled
Comment #29
dawehner#28: vdc-2027115-28.patch queued for re-testing.
Comment #30
dawehnerAdded some tests and removed that todo.
Comment #32
dawehner.
Comment #34
tim.plunkett#32: override-2027115-32.patch queued for re-testing.
Comment #36
dawehner#32: override-2027115-32.patch queued for re-testing.
Comment #37
jibranFinally it is green. I think we should add issues for all @todo's introduced by this patch. This is a critical issue so adding a needs followup tag.
Yay!!
Comment #38
dawehnerTo be honest I don't have the feeling that we should get this in as it is.
One major problem is that we should keep the existing route names otherwise the url generator will just fail.
Comment #39
tim.plunkettWe need to get this in to continue converting listings to views, and page callbacks to routes. Non-critical follow-ups are great
Comment #40
jibran+1 RTBC and what @tim.plunkett said.
Comment #41
dawehnerWell 2 against 1 but I would really think that my last point is critical. I can't explain at that point of the day why exactly.
Comment #42
webchickHm. I agree with Daniel. Given that we are starting to move to things like e.g. confirm forms acting on routes rather than paths, it seems like we should really figure this out pre-commit, since it could change the implementation. No?
Comment #43
dawehnerSo just take one example you want to link to admin/people and admin/people would be already a view.
With the patch applied the generated route name of the view would differ from the one provided by default.
I will work on this tomorrow so we somehow keep the old route name.
Comment #44
dawehnerComment #46
dawehnerJust tracking some work, note this is now horrible broken as work in progress.
Comment #47
AjitSI think you missed to change the status to needs review.
Comment #49
damiankloip CreditAttribution: damiankloip commentedNo, I think that was intentional :-)
Comment #49.0
damiankloip CreditAttribution: damiankloip commentedblub
Comment #50
dawehnerRealized that the access subscriber comes to early, so moved down the priority.
Comment #51
dawehner.
Comment #52
dawehner.
Comment #54
damiankloip CreditAttribution: damiankloip commentedSuspect comment.
Same.
We're already in the namespace, so do we need it here? don't we usually just use class name.
Full stop.
This could live on one of the views classes we already have maybe? (follow up)
Could we mapValuesToKeys() here instead? would be easier reading.
Maybe that method should return a keyed array instead? thoughts?
Follow up?
Why do we need to check this again? (I think we talked about this before, so it's again again).
If you're using setDisplay() you could just get the display directly from $view->display_handler, as the get from the display bag is already done.
Can we add more brackets here, it's easier on my little eyes. If you don't want to though, that's cool too ;)
From my experience in admin_views, the answer is yes. We can talk about that though, should be some followup.
Comment #55
dawehnerI went this way because everything in core is called RouteSubscriber but I so much don't care but too lazy currently to change it and rather write an incredible long sentence without a point in there.
Yeah something like a view-repository could make sense? I can't think of another service where this really makes sense at the moment.
Well, then the mapArray::copyValuesToKeys won't work anymore.
#2102293: Inject the view executable factory where possible
Because I need autocompletion :P
Good point, though this is kind of internal behavior, isn't it? A method is nicer and can be actually unit tested. (What about a follow up to create an getter for the display_handler like getCurrentDisplay() ?
Serializer test is not a unit tests, arg!
Comment #56
damiankloip CreditAttribution: damiankloip commentedYes, if we went down that route obviously that part would go too :-) I had that thought after.
Comment #57
tim.plunkettUsing state for this is a really good idea, +1.
The unit tests look perfect.
And we even prove it works for user_admin_account()!
Comment #58
webchickSorry, only nitpicks for now, I got called away to something else.
an excutable
That PHPDoc has to be right on top of the interface, else api.d.o doesn't pick it up.
Docs gate.
(nitpick) indentation.
Comment #59
tim.plunkettsetUp() still doesn't get a docblock, per our standards.
Fixed the rest
Comment #60
jibranBack to RTBC as #58 is addressed.
Comment #61
dawehnerOne small fix to not let the add "user local action" appear multiple times.
Comment #62
dawehner#61: override-2027115-61.patch queued for re-testing.
Comment #63
webchickSat down to review this more in detail today. I'm just going to be bluntly honest and say that most of this patch is way over my head since it involves both gnarly views internals and gnarly routing system internals. OTOH, I think it's really important to make progress on getting the routing system done, and if there are problems that are introduced here, they can likely be cleaned up elsewhere while in the meantime we can make progress.
In reviewing, I didn't find any blocking concerns, so...
Committed and pushed to 8.x. Thanks!
Comment #63.0
webchickblub
Comment #64
dawehner#2110845: Allow overriding views to override paths with parameters is a follow up.
Comment #65.0
(not verified) CreditAttribution: commentedblub