Problem/Motivation

For entity routes using an entity route provider is superior to declaring the routes statically as they can be more easily altered, for example.

Proposed resolution

Replace contact_storage.routing.yml with a MessageRouteProvider.

Remaining tasks

User interface changes

API changes

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tstoeckler’s picture

Title: Use an entity route provider instead of a static routing YAML fil » Use an entity route provider instead of a static routing YAML file
Status: Active » Needs review
FileSize
4 KB

While moving the definitions over I also fixed them to use _entity_access instead of _permission directly. Everything else should be unchanged.

tstoeckler’s picture

FileSize
933 bytes
4.02 KB

Sometimes it really helps to actually try things...

tstoeckler--

I sincerely hope #1 is red, otherwise we need some tests for this...

The last submitted patch, 1: 2476591-message-route-provider.patch, failed testing.

andypost’s picture

Issue tags: +Needs tests
Berdir’s picture

Status: Needs review » Needs work
Issue tags: -Needs tests
+++ b/src/MessageRouteProvider.php
@@ -0,0 +1,65 @@
+ * Provides routes for contact messages.
+ */
+class MessageRouteProvider implements EntityRouteProviderInterface {
+

Can we switch to the default html route provider now and check how much we can remove?

#1 was red, I think we have existing tests that cover this?

larowlan’s picture

Issue tags: +Novice, +php-novice

Bumping

Bambell’s picture

The patch wouldn't apply, so I re-rolled it. All static routes are removed (contact_storage.routing.yml) and I added the missing code to contact_storage.module and MessageRouteProvider.php.

Bambell’s picture

Status: Needs work » Needs review
Berdir’s picture

Status: Needs review » Needs work
  1. +++ b/src/MessageRouteProvider.php
    @@ -0,0 +1,89 @@
    +
    +/**
    + * @file
    + * Contains \Drupal\contact_storage\MessageRouteProvider.
    + */
    +
    

    no longer needed.

  2. +++ b/src/MessageRouteProvider.php
    @@ -0,0 +1,89 @@
    + * Provides routes for contact messages.
    + */
    +class MessageRouteProvider implements EntityRouteProviderInterface {
    +
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function getRoutes(EntityTypeInterface $entity_type) {
    

    Extend from DefaultHtmlRouteProvider, that should cover edit, delete and canonical.

Bambell’s picture

Bambell’s picture

Status: Needs work » Needs review
Berdir’s picture

Status: Needs review » Needs work
+++ b/src/MessageRouteProvider.php
@@ -71,16 +42,16 @@ class MessageRouteProvider implements EntityRouteProviderInterface {
 
+    if ($entity_type->hasLinkTemplate('edit-form')) {
+      $route_collection->add('entity.' . $entity_type->id() . '.edit_form', $this->getEditFormRoute($entity_type));
+    }

Just call $route_collection = parent::getRoutes() first, then you can drop those parts completely.

Bambell’s picture

Right. I should have better looked at DefaultHtmlRouteProvider.

Bambell’s picture

Status: Needs work » Needs review
andypost’s picture

$route_providers = $entity_types['contact_form']->getRouteProviderClasses();
+ $route_providers['html'] = '\Drupal\contact_storage\MessageRouteProvider';
+ $entity_types['contact_form']->setHandlerClass('route_provider', $route_providers);

duplicated twice in patch?!

Bambell’s picture

Need to define route providers for both contact_form and contact_storage entities, but it does appear to be the same in both cases, so I made some changes..

Berdir’s picture

Status: Needs review » Needs work
+++ b/contact_storage.module
@@ -179,6 +179,13 @@ function contact_storage_entity_type_alter(array &$entity_types) {
+  $route_providers = $entity_types['contact_message']->getRouteProviderClasses();
+  $route_providers['html'] = '\Drupal\contact_storage\MessageRouteProvider';
+  $entity_types['contact_message']->setHandlerClass('route_provider', $route_providers);
+  $entity_types['contact_form']->setHandlerClass('route_provider', $route_providers);

Hm. Given that we use the same, it is a bit weird that we call it MessageRouteProvider, maybe that confused @andypost a bit?

What about renaming to ContactRouteProvider?

Then we are ready I think.

Bambell’s picture

Sure. I renamed the class to ContactRouteProvider.

Bambell’s picture

Status: Needs work » Needs review
Berdir’s picture

Status: Needs review » Reviewed & tested by the community

Thank you.

  • andypost committed eb46ddf on 8.x-1.x authored by Bambell
    Issue #2476591 by Bambell, tstoeckler: Use an entity route provider...
andypost’s picture

Assigned: tstoeckler » Unassigned
Status: Reviewed & tested by the community » Fixed

Thanx, commited and pushed

tstoeckler’s picture

Yay! Thanks @all for pushing this forward and getting it done!

Bambell++

Status: Fixed » Closed (fixed)

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