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.

User interface changes

API changes

Original report by @dawehner

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

Status: Active » Needs review
FileSize
22.65 KB

The 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.

Status: Needs review » Needs work

The last submitted patch, vdc-2027115-1.patch, failed testing.

tim.plunkett’s picture

FileSize
18.24 KB

Rerolled without the change to the view yaml.

tim.plunkett’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, vdc-2027115-3.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
18.24 KB
666 bytes

666

Status: Needs review » Needs work

The last submitted patch, vdc-2027115-7.patch, failed testing.

tim.plunkett’s picture

Drupal\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

dawehner’s picture

Status: Needs work » Needs review
FileSize
21.64 KB

Even 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.

Status: Needs review » Needs work

The last submitted patch, vdc-2027115-10.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
6.51 KB
25.38 KB

Just tracking some work.

Status: Needs review » Needs work
Issue tags: -VDC

The last submitted patch, vdc-2027115-12.patch, failed testing.

apaderno’s picture

Status: Needs work » Needs review

#12: vdc-2027115-12.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +VDC

The last submitted patch, vdc-2027115-12.patch, failed testing.

effulgentsia’s picture

dawehner’s picture

Status: Needs work » Needs review
FileSize
25.38 KB

Ha, this works way better!

Status: Needs review » Needs work

The last submitted patch, vdc-2027115-17.patch, failed testing.

jibran’s picture

  1. +++ b/core/modules/user/lib/Drupal/user/Controller/UserAdmin.php
    @@ -0,0 +1,161 @@
    +  protected $query;
    

    this should be entityQuery.

  2. +++ b/core/modules/user/lib/Drupal/user/Controller/UserAdmin.php
    @@ -0,0 +1,161 @@
    +    $roles = array_map('Drupal\Component\Utility\String::checkPlain', user_role_names(TRUE));
    

    Can we add this namespace above or \ before namespace?

  3. +++ b/core/modules/views/lib/Drupal/views/Entity/View.php
    @@ -320,6 +320,17 @@ public function getExportProperties() {
       /**
        * {@inheritdoc}
    +   *
    +   * Figures out which routes are overridden l
    +   * @param EntityStorageControllerInterface $storage_controller
    

    We can't mix @inheritdoc and description. Blank line is missing after the comment. @param description is missing.

  4. +++ b/core/modules/views/lib/Drupal/views/EventSubscriber/RouteSubscriber.php
    @@ -18,14 +20,55 @@
    +   * @param \Drupal\Core\Entity\EntityManager $entity_manager
    
    +++ b/core/modules/views/lib/Drupal/views/Plugin/views/display/DisplayRouterInterface.php
    @@ -25,4 +26,12 @@
    +   * @param \Symfony\Component\Routing\RouteCollection $collection
    

    @param description is missing.

  5. +++ b/core/modules/views/tests/Drupal/views/Tests/Plugin/display/PathPluginBaseTest.php
    @@ -0,0 +1,74 @@
    +  public static function getInfo() {
    

    doc block missing.

  6. +++ b/core/modules/views/views.services.yml
    @@ -83,7 +83,7 @@ services:
    +    arguments: ['@plugin.manager.entity']
    

    entity.manager now

dawehner’s picture

Status: Needs work » Needs review
FileSize
6.16 KB
26.49 KB

Thank you for the review. Let's see how much passes now.

Status: Needs review » Needs work

The last submitted patch, vdc-2027115-20.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
21.08 KB
29.31 KB

Some bugfixes here and there.

dawehner’s picture

This interdiff seems horrible.

damiankloip’s picture

  1. +++ b/core/modules/user/user.module
    @@ -880,6 +882,22 @@ function user_menu() {
    + * Implements hook_local_actions().
    + */
    +function user_local_actions() {
    

    Can't we just add a user.local_actions.yml file instead?

  2. +++ b/core/modules/views/lib/Drupal/views/EventSubscriber/RouteSubscriber.php
    @@ -18,14 +20,63 @@
    +   * Reset the internal state of the route subscriber
    

    Resets

  3. +++ b/core/modules/views/lib/Drupal/views/EventSubscriber/RouteSubscriber.php
    @@ -18,14 +20,63 @@
    +    unset($this->viewsDisplayPairs);
    

    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.

  4. +++ b/core/modules/views/lib/Drupal/views/EventSubscriber/RouteSubscriber.php
    @@ -18,14 +20,63 @@
    +      $views = views_get_applicable_views('uses_route');
    

    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.

  5. +++ b/core/modules/views/lib/Drupal/views/EventSubscriber/RouteSubscriber.php
    @@ -34,15 +85,43 @@ public static function getSubscribedEvents() {
    +      if (($view = $view->getExecutable()) && $view instanceof ViewExecutable) {
    ...
    +      if (($view = $view->getExecutable()) && $view instanceof ViewExecutable) {
    

    Why the check for ViewExecutable here?

  6. +++ b/core/modules/views/lib/Drupal/views/Plugin/views/display/DisplayRouterInterface.php
    @@ -25,4 +26,12 @@
    +   * @param \Symfony\Component\Routing\RouteCollection $collection
    +   *
    +   * @return bool
    +   *   Returns TRUE if an existing route was found, otherwise FALSE.
    

    Needs a first doc line, I think the @return is wrong too.

  7. +++ b/core/modules/views/lib/Drupal/views/Plugin/views/display/PathPluginBase.php
    @@ -20,6 +23,36 @@
    +   *   The route provider
    

    full stop.

  8. +++ b/core/modules/views/lib/Drupal/views/Plugin/views/display/PathPluginBase.php
    @@ -60,12 +93,17 @@ protected function defineOptions() {
    +   * Generate a route entry for a given view and display.
    

    Generates

  9. +++ b/core/modules/views/lib/Drupal/views/Plugin/views/display/PathPluginBase.php
    @@ -126,11 +164,44 @@ public function collectRoutes(RouteCollection $collection) {
    +  public function alterRoutes(RouteCollection $collection) {
    

    So alter routes just runs after collectRoutes has run and basically cleans up (removes) and routes that will be overridden by view?

  10. +++ b/core/modules/views/tests/Drupal/views/Tests/Plugin/display/PathPluginBaseTest.php
    @@ -0,0 +1,76 @@
    +    $view = $this->getMockBuilder('Drupal\views\ViewExecutable')
    +      ->disableOriginalConstructor()
    +      ->getMock();
    +    $display = array();
    +    $display['display_plugin'] = 'page';
    +    $display['id'] = 'page_1';
    +    $display['display_options'] = array(
    

    Is the plan there to set that on the executable? Because that could be problematic if you need to use properties directly.

  11. +++ b/core/modules/views/tests/Drupal/views/Tests/Plugin/display/PathPluginBaseTest.php
    @@ -0,0 +1,76 @@
    +    // @todo write the actual test.
    

    I guess this needs to be done still :)

dawehner’s picture

FileSize
3.77 KB
29.24 KB

Why the check for ViewExecutable here?

I had some issues while writing the patch, so this worked ...

So alter routes just runs after collectRoutes has run and basically cleans up (removes) and routes that will be overridden by view?

Exactly, but yes, this is actually not the right approach. You would have to keep the existing route name to work as expected.

I guess this needs to be done still :)

Not at all ;)

dawehner’s picture

FileSize
29.6 KB

Missing file.

Status: Needs review » Needs work

The last submitted patch, vdc-2027115-26.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
29.55 KB

Rerolled

dawehner’s picture

#28: vdc-2027115-28.patch queued for re-testing.

dawehner’s picture

FileSize
3.17 KB
29.67 KB

Added some tests and removed that todo.

Status: Needs review » Needs work
Issue tags: -VDC

The last submitted patch, route-2027115-30.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
857 bytes
29.64 KB

.

Status: Needs review » Needs work
Issue tags: -blocker

The last submitted patch, override-2027115-32.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review

#32: override-2027115-32.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, override-2027115-32.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
Issue tags: +blocker

#32: override-2027115-32.patch queued for re-testing.

jibran’s picture

Issue tags: +Needs followup

Finally 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.

+++ b/core/modules/user/user.local_actions.yml
@@ -0,0 +1,6 @@
+    - view.user_admin_people.page_1

Yay!!

dawehner’s picture

To 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.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

We need to get this in to continue converting listings to views, and page callbacks to routes. Non-critical follow-ups are great

jibran’s picture

+1 RTBC and what @tim.plunkett said.

dawehner’s picture

One major problem is that we should keep the existing route names otherwise the url generator will just fail.

Well 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.

webchick’s picture

Status: Reviewed & tested by the community » Needs review

Hm. 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?

dawehner’s picture

So 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.

dawehner’s picture

FileSize
17.92 KB
40.15 KB
  • Implemented the logic to keep the existing route name if we alter it
  • Wrote unit tests for all the things.

Status: Needs review » Needs work

The last submitted patch, override-2027115-44.patch, failed testing.

dawehner’s picture

FileSize
42.63 KB
11.53 KB

Just tracking some work, note this is now horrible broken as work in progress.

AjitS’s picture

Status: Needs work » Needs review

I think you missed to change the status to needs review.

Status: Needs review » Needs work

The last submitted patch, override-2027115-46.patch, failed testing.

damiankloip’s picture

No, I think that was intentional :-)

damiankloip’s picture

Issue summary: View changes

blub

dawehner’s picture

FileSize
2.48 KB
44.3 KB

Realized that the access subscriber comes to early, so moved down the priority.

dawehner’s picture

Status: Needs work » Needs review

.

dawehner’s picture

.

Status: Needs review » Needs work

The last submitted patch, override-2027115-50.patch, failed testing.

damiankloip’s picture

  1. +++ b/core/modules/views/lib/Drupal/views/EventSubscriber/RouteSubscriber.php
    @@ -7,25 +7,101 @@
    + * The general idea done here is to first execute
    

    Suspect comment.

  2. +++ b/core/modules/views/lib/Drupal/views/EventSubscriber/RouteSubscriber.php
    @@ -7,25 +7,101 @@
    +   * Stores a list of view,display IDs containing routes and still has to be
    +   * added.
    

    Same.

  3. +++ b/core/modules/views/lib/Drupal/views/EventSubscriber/RouteSubscriber.php
    @@ -7,25 +7,101 @@
    +   * Constructs a \Drupal\views\EventSubscriber\RouteSubscriber instance.
    

    We're already in the namespace, so do we need it here? don't we usually just use class name.

  4. +++ b/core/modules/views/lib/Drupal/views/EventSubscriber/RouteSubscriber.php
    @@ -7,25 +7,101 @@
    +   * Resets the internal state of the route subscriber
    

    Full stop.

  5. +++ b/core/modules/views/lib/Drupal/views/EventSubscriber/RouteSubscriber.php
    @@ -7,25 +7,101 @@
    +      // @todo Convert this method to some service.
    

    This could live on one of the views classes we already have maybe? (follow up)

  6. +++ b/core/modules/views/lib/Drupal/views/EventSubscriber/RouteSubscriber.php
    @@ -7,25 +7,101 @@
    +        $this->viewsDisplayPairs[$id . '.' . $display_id] = $id . '.' . $display_id;
    

    Could we mapValuesToKeys() here instead? would be easier reading.

  7. +++ b/core/modules/views/lib/Drupal/views/EventSubscriber/RouteSubscriber.php
    @@ -34,16 +110,67 @@ public static function getSubscribedEvents() {
    +    foreach ($this->getViewsDisplayIDsWithRoute() as $pair) {
    +      list($view_id, $display_id) = explode('.', $pair);
    

    Maybe that method should return a keyed array instead? thoughts?

  8. +++ b/core/modules/views/lib/Drupal/views/EventSubscriber/RouteSubscriber.php
    @@ -34,16 +110,67 @@ public static function getSubscribedEvents() {
    +      // @todo This should have a executable factory injected.
    

    Follow up?

  9. +++ b/core/modules/views/lib/Drupal/views/EventSubscriber/RouteSubscriber.php
    @@ -34,16 +110,67 @@ public static function getSubscribedEvents() {
    +      if (($view = $view->getExecutable()) && $view instanceof ViewExecutable) {
    

    Why do we need to check this again? (I think we talked about this before, so it's again again).

  10. +++ b/core/modules/views/lib/Drupal/views/EventSubscriber/RouteSubscriber.php
    @@ -34,16 +110,67 @@ public static function getSubscribedEvents() {
    +        if ($view->setDisplay($display_id) && $display = $view->displayHandlers->get($display_id)) {
    

    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.

  11. +++ b/core/modules/views/lib/Drupal/views/Plugin/views/display/PathPluginBase.php
    @@ -126,8 +185,50 @@ public function collectRoutes(RouteCollection $collection) {
    +      if (!$route->hasDefault('view_id') &&  '/' . $view_path == $route_path) {
    

    Can we add more brackets here, it's easier on my little eyes. If you don't want to though, that's cool too ;)

  12. +++ b/core/modules/views/lib/Drupal/views/Plugin/views/display/PathPluginBase.php
    @@ -126,8 +185,50 @@ public function collectRoutes(RouteCollection $collection) {
    +        // @todo Figure out whether we need to merge some settings (like
    +        // requirements).
    

    From my experience in admin_views, the answer is yes. We can talk about that though, should be some followup.

dawehner’s picture

Status: Needs work » Needs review
FileSize
9.07 KB
45.53 KB

We're already in the namespace, so do we need it here? don't we usually just use class name.

I 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.

This could live on one of the views classes we already have maybe? (follow up)

Yeah something like a view-repository could make sense? I can't think of another service where this really makes sense at the moment.

Maybe that method should return a keyed array instead? thoughts?

Well, then the mapArray::copyValuesToKeys won't work anymore.

Follow up?

#2102293: Inject the view executable factory where possible

Why do we need to check this again?

Because I need autocompletion :P

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.

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!

damiankloip’s picture

Well, then the mapArray::copyValuesToKeys won't work anymore.

Yes, if we went down that route obviously that part would go too :-) I had that thought after.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Using state for this is a really good idea, +1.

The unit tests look perfect.

And we even prove it works for user_admin_account()!

webchick’s picture

Status: Reviewed & tested by the community » Needs work

Sorry, only nitpicks for now, I got called away to something else.

+++ b/core/modules/views/lib/Drupal/views/EventSubscriber/RouteSubscriber.php
@@ -34,16 +114,67 @@ public static function getSubscribedEvents() {
+      // @todo This should have a executable factory injected.

an excutable

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/display/DisplayRouterInterface.php
@@ -7,13 +7,14 @@
  */
-use Symfony\Component\Routing\RouteCollection;
 
 interface DisplayRouterInterface {

That PHPDoc has to be right on top of the interface, else api.d.o doesn't pick it up.

+++ b/core/modules/views/tests/Drupal/views/Tests/EventSubscriber/RouteSubscriberTest.php
@@ -0,0 +1,209 @@
+  public static function getInfo() {
...
+  protected function setupMocks() {
...
+class TestRouteSubscriber extends RouteSubscriber {

Docs gate.

+++ b/core/modules/views/tests/Drupal/views/Tests/Plugin/display/PathPluginBaseTest.php
@@ -0,0 +1,187 @@
+  use Drupal\Core\DependencyInjection\ContainerBuilder;
+  use Drupal\Tests\UnitTestCase;
+use Symfony\Component\Routing\Route;
+use Symfony\Component\Routing\RouteCollection;

(nitpick) indentation.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
4.34 KB
45.84 KB

setUp() still doesn't get a docblock, per our standards.

Fixed the rest

jibran’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC as #58 is addressed.

dawehner’s picture

Issue tags: +VDC
FileSize
879 bytes
45.88 KB

One small fix to not let the add "user local action" appear multiple times.

dawehner’s picture

Issue tags: -VDC

#61: override-2027115-61.patch queued for re-testing.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Sat 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!

webchick’s picture

Issue summary: View changes

blub

dawehner’s picture

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

Issue summary: View changes

blub