Make Vocabulary use a route provider instead of hard-coded routes

YAML file translated to RouteProvider

Thanks for getting the ball rolling on this effort again! Adding parent issue.

Also looked at the actual patch. Really nice work! I do have a number of comments, though, most of them are just minor cleanup, though.

1. +++ b/core/modules/taxonomy/src/Entity/Routing/VocabularyRouteProvider.php
   @@ -0,0 +1,87 @@
   +    if ($overview_page_route = $this->getOverviewPageRoute($entity_type)) {
   +      $collection->add("entity.taxonomy_vocabulary.overview_form", $overview_page_route);
   +    }
   

   It's unfortunate that this route is in the "taxonomy_vocabulary" namespace as, conceptually, I think it matches more closely to a term collection route. However, I am not sure we can properly generate a collection route which depends on the bundle anyway, so I think it makes most sense to do exactly what you did.

2. +++ b/core/modules/taxonomy/src/Entity/Routing/VocabularyRouteProvider.php
   @@ -0,0 +1,87 @@
   +      $route->setDefault('_title', 'Add vocabulary');
   

   The generic title being used here is "Add @lowercase_entity_label".
   It is unfortunate that the label of the Vocabulary entity type is (incorrectly, in my opinion) "Taxonomy vocabulary", so the corresponding lowercase label would be "taxonomy vocabulary". So I think it's correct to override the title here. Alternatively we could change the label to just "Vocabulary" but I guess that could be considered a break of backwards-compatibility. So I think this is exactly right. (Also note #2507235: EntityType::getLowercaseLabel() breaks translation)

3. +++ b/core/modules/taxonomy/src/Entity/Routing/VocabularyRouteProvider.php
   @@ -0,0 +1,87 @@
   +      $route->setDefault('_title', 'Taxonomy');
   

   Again, the generic label is determined by the collection label in the annotation. I could see an argument that "a collection of vocabularies" is synonymous with "Taxonomy" (if not in the generic meaning of those words, maybe that is true at least in Drupal). If so, we could simply add a collection label to vocabularies and drop this override. Not 100% sure, though.

4. +++ b/core/modules/taxonomy/src/Entity/Routing/VocabularyRouteProvider.php
   @@ -0,0 +1,87 @@
   +    $route->setRequirement('_permission', 'administer taxonomy');
   

   For extra credit we could use $entity_type->getAdminPermission() here to maybe this just a tad less hardcoded.

5. +++ b/core/modules/taxonomy/src/Entity/Routing/VocabularyRouteProvider.php
   @@ -0,0 +1,87 @@
   +   * Gets the reset page route.
   ...
   +  protected function getOverviewPageRoute(EntityTypeInterface $entity_type) {
   

   Copy-paste error.

6. +++ b/core/modules/taxonomy/src/Entity/Vocabulary.php
   @@ -12,14 +12,25 @@
   + *       "add" = "Drupal\taxonomy\VocabularyForm",
   

   I think - even though, as far as I can tell, this is not strictly required for this patch - it makes sense to include this as part of this patch. But why not also add an "edit" form operation, as well, in that case?

7. +++ b/core/modules/taxonomy/src/Entity/Vocabulary.php
   @@ -12,14 +12,25 @@
   + *       "overview" = "Drupal\taxonomy\Form\OverviewTerms"
   

   This makes a lot of sense!

8. +++ b/core/modules/taxonomy/src/Entity/Vocabulary.php
   @@ -31,7 +42,7 @@
   - *     "add-form" = "/admin/structure/taxonomy/manage/{taxonomy_vocabulary}/add",
   + *     "add-form" = "/admin/structure/taxonomy/add",
   

   Nice bugfix!

9. +++ b/core/modules/taxonomy/taxonomy.routing.yml
   @@ -1,11 +1,3 @@
   -    _permission: 'access taxonomy overview+administer taxonomy'
   

   The double-permissions are not covered by the auto-generated route, so for now we will have to override this. I have also hit this a bunch of times and have thought about whether we need some sort of "collection permission" in addition to the admin permission, but for now let's just do an override.

10. +++ b/core/modules/taxonomy/taxonomy.routing.yml
    @@ -36,46 +28,6 @@ entity.taxonomy_term.delete_form:
    -    _title_callback: '\Drupal\taxonomy\Controller\TaxonomyController::vocabularyTitle'
    

    This is currently not covered by the route provider. Not sure, whether it is actually needed to port this, though. Maybe we can get some before/after screenshots of the taxonomy edit page title and then get some usability feedback on whether this change is acceptable (I would argue it's even an improvement) Please also include some HTML in the vocabulary label, as the current edit page label is quite weird in that it outputs the vocabulary label as HTML. With the generic label the page title just becomes "Edit %label".

11. +++ b/core/modules/taxonomy/taxonomy.routing.yml
    @@ -36,46 +28,6 @@ entity.taxonomy_term.delete_form:
    -    _title: 'Delete vocabulary'
    

    Again, this is now changed to "Delete %label" with the generic route, but I would argue that this is actually a nice little improvement. I do think we need confirmation by the usability team, though.

Thanks for the detailed review @tstoeckler!

I set out to translate the YML file as closely as I could and clean up a few things that I found (like that bug). So while I think it's great to take this opportunity to identify areas for improvement, perhaps it's best to turn those into new issues? I'll take guidance on that of course because this is my first time working on core.

1. Yeah, I wouldn't know where to begin!
2. Agreed.
3. Yeah backwards compatibility and user familiarity are the biggest issues I see here.
4. Nice, done for the reset page - this is new to me, should we be using this elsewhere?
5. Oops
6. Ah, that's left over from when I was trying to figure out why I was getting 403s (see #8), it's not necessary. I've removed it for now, happy to add that and the edit form link back in if there's a good enough reason.
7. It's required
8. Thanks, found it by accident as it was causing me a huge headache. I would make it a separate issue but this patch won't work without the fix and it's a single line so... ¯\_(ツ)_/¯
9. You're right, I missed that and it caused one of the failed tests.
10. For now I've just pointed the route provider at that same method since translation is my main goal. This also caused a failed test.
11. For completeness of approach I've added the override similar to the add form. Again, further guidance is more than welcome, I'm still learning about route providers.

I've fixed the three failed tests. I hope.

Thanks for the fixes!

+++ b/core/modules/taxonomy/src/Entity/Routing/VocabularyRouteProvider.php
@@ -78,7 +99,7 @@ protected function getOverviewPageRoute(EntityTypeInterface $entity_type) {
-    $route->setRequirement('_entity_access', 'taxonomy_vocabulary.view');
+    $route->setRequirement('_entity_access', 'taxonomy_vocabulary.access taxonomy overview');

WTF?! I would totally have bet money on the fact that that doesn't work and is totally broken...

...but I see that you've just taken that over from the YAML file. Nice catch! (Still looks quite bizarre, due to the spaces in the string.)

So I really have nothing to complain about with your patch, it looks perfect. Just one note about your answers in #9:

3. Yeah backwards compatibility and user familiarity are the biggest issues I see here.

So what I was thinking was to add label_collection = @Translation("Taxonomy") to the entity type annotation which would alleviate the need to override the title here. That is definitely not a BC-break, the question is rather whether that is actually the correct "collection label". Having thought about this a bit, I think the answer is yes. However, I think it's totally fine to leave the patch as is, and consider doing that in a separate issue.

On a more general note, and somewhat in reply to your points 10. and 11.: When working on other route providers for core before, I have experienced core committers not being to fond of having all these overrides in the route providers. That is why I mentioned that it might make sense to introduce small UI changes (potentially even improvements) so that the route provider becomes a little less heavy.

But let's find out what the committers say about this. For me, the patch looks good to go as is. If the test bot doesn't complain, I will mark this RTBC.

When working on other route providers for core before, I have experienced core committers not being to fond of having all these overrides in the route providers. That is why I mentioned that it might make sense to introduce small UI changes (potentially even improvements) so that the route provider becomes a little less heavy.

Yes in a minor release we can make small string changes, so if the defaults provided by the route provider are fine (or an improvement), then I think we should drop the overrides here - this will slowly make entity type management in the UI more consistent as well. CNR for that - we might need screenshots for those UI changes though just to see what's going on.

OK, so concrete suggestions:

1. Remove the overrides for the add, edit and delete form routes. This changes the titles on the respective pages, so we will need before/after screenshots for all of these. Please use a vocabulary with HTML in the label for this.
2. Add a label_collection and remove the _title override for the collection route. (No UI change)

This will leave the permission override for the collection route as the only override. The route provider will then still contain the two additional vocabulary-specific routes.

@catch, does that sound acceptable?

Findings:

1. The title override for the add and delete pages is unnecessary - the output is exactly the same both before and after so there's only one screenshot:
   
2. The breadcrumbs are fine as long as we add the labels in annotations, without them we get:
   
   
3. Removing the title override for the edit page does affect sub-pages as expected.
   Before:
   
   After:
   
4. It also breaks this test:
   

   <?php
     /**
      * Tests breadcrumbs on node and administrative paths.
      */
     public function testBreadCrumbs() {
       // Prepare common base breadcrumb elements.
       $home = ['' => 'Home'];
       $admin = $home + ['admin' => t('Administration')];
       $config = $admin + ['admin/config' => t('Configuration')];
       $type = 'article';
   
       // Verify Taxonomy administration breadcrumbs.
       $trail = $admin + [
         'admin/structure' => t('Structure'),
       ];
       $this->assertBreadcrumb('admin/structure/taxonomy', $trail);
   
       $trail += [
         'admin/structure/taxonomy' => t('Taxonomy'),
       ];
       $this->assertBreadcrumb('admin/structure/taxonomy/manage/tags', $trail);
       $trail += [
         'admin/structure/taxonomy/manage/tags' => t('Tags'),
       ];
       $this->assertBreadcrumb('admin/structure/taxonomy/manage/tags/overview', $trail);

   
   The test is expecting "Tags" at the end of the breadcrumb and receives "Edit Tags"

So my question is should I update the test?

Assuming the answer is affirmative, here's the patch and interdiff including the change to the test (because otherwise there will definitely be a failure). Let's see what the testbot makes of it.

Yes, the answer is in fact affirmative ;-)

I did not realize that both VocabularyForm and EntityConfirmFormBase use #title to set the page title, therefore the changes are less invasive than I thought. The screenshots in #15.3 show the only visible change: the breadcrumb. Note that this is not a double-escape bug, but the label of the vocabulary is literally "<em>Fluffy</em> Stuff". In current HEAD, the label is actually put as HTML into the breadcrumb.

As far as I can tell, the issue summary was updated above already, so going back to RTBC.

Edit: Well, of course trying to show HTML in a textarea that accepts HTML as input is not really smart. Next try.

Committed/pushed to 8.5.x, thanks!

Thanks for the quick turnaround @catch! The way this issue has gone makes me hopeful that we can we make some more progress on route providers.

this will slowly make entity type management in the UI more consistent as well

Thinking about this more lead me to open #2924408: Deprecate TaxonomyController::vocabularyTitle().