Doing this after #2098795: Create Base Class for RouteSubscriber Class - Done

We are still using module in the RouteBuildEvent class, this doesn't make sense as we are now using 'provider' everywhere else pretty much. Especially as we have 'dynamic_routes' as the current 'module' when altering the dynamic route collection, also, doesn't make sense. Provider this works better.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

damiankloip’s picture

Status: Postponed » Needs review
FileSize
4.05 KB
damiankloip’s picture

Title: change Drupal\Core\Routing\RouteBuildEvent::getModule() to getProvider » Change Drupal\Core\Routing\RouteBuildEvent::getModule() to getProvider()

Status: Needs review » Needs work

The last submitted patch, 2102417-1.patch, failed testing.

damiankloip’s picture

Assigned: damiankloip » Unassigned
Status: Needs work » Needs review
FileSize
649 bytes
4.33 KB

Doh, forgot to add the remaining changes to the last patch out.

Crell’s picture

+1 on this in concept. Although I think the Dumper uses "route set" or something generic like that. We should probably use the same term there as well. (Either switch to "set" here, or "provider" there. I guess I'm OK with either.)

damiankloip’s picture

FileSize
6.2 KB
11.68 KB

That sounds sensible, I think we should go with provider, as this term is being used a fair bit in D8 now.

Status: Needs review » Needs work

The last submitted patch, 2102417-6.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
944 bytes
11.75 KB

Gah, update schema meh.

dawehner’s picture

Just wondering whether it is worth to break other peoples code, if they use the actual RouteSubscriberInterface.

  1. +++ b/core/lib/Drupal/Core/Routing/MatcherDumper.php
    @@ -77,8 +77,8 @@ public function addRoutes(RouteCollection $routes) {
    +   * - provider:  The route grouping that is being dumped. All existing
    

    I would be fine with having a little bet less spaces after the ":"

  2. +++ b/core/lib/Drupal/Core/Routing/RouteSubscriberBase.php
    @@ -20,7 +20,7 @@
    -   * objects with $collection->add($route);.
    +   * objects with $collection->add('route_name', $route);.
    

    +2!

  3. +++ b/core/modules/system/system.install
    @@ -2295,6 +2295,22 @@ function system_update_8060() {
     /**
    + * Rename the router table 'route_set' field to 'provider'.
    + */
    +function system_update_8061() {
    

    Do we already provide inter-drupal updates?

Crell’s picture

No inter-drupal updates at this point.

damiankloip’s picture

FileSize
9.61 KB

Removed the update hook from system.install, was more of a reflex bit of coding :) Also changed that one piece of whitespace. Apart from that, patch is the same.

I think this is definitely worth the change, as making our various APIs more consistent is pretty important I think. If in some cases you are using providers and some modules or route sets, this is much much worse.

Status: Needs review » Needs work

The last submitted patch, 2102417-11.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review

#11: 2102417-11.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 2102417-11.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
2.68 KB
11.68 KB

Oops, didn't want to remove all the things from system.install - that was rash.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

There is nothing in core anymore which is called 'route_set', so yeah!

dawehner’s picture

Issue summary: View changes

Updated issue summary.

alexpott’s picture

git ac https://drupal.org/files/2102417-15.patch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 11957  100 11957    0     0   5908      0  0:00:02  0:00:02 --:--:--  6487
error: patch failed: core/lib/Drupal/Core/Routing/RouteBuilder.php:84
error: core/lib/Drupal/Core/Routing/RouteBuilder.php: patch does not apply
damiankloip’s picture

Status: Needs work » Needs review
FileSize
11.64 KB

Rerolled

Status: Needs review » Needs work

The last submitted patch, 18: 2102417-18.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
13.61 KB
1.25 KB

Need to fix the new RouteBuilder tests..This should be good now.

damiankloip’s picture

FileSize
12.9 KB

And without the random newline added in the node revision access checker.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Back to rtbc

alexpott’s picture

+++ b/core/lib/Drupal/Core/Routing/RouteBuilder.php
@@ -119,7 +119,7 @@ public function rebuild() {
-    $this->dumper->dump(array('route_set' => 'dynamic_routes'));
+    $this->dumper->dump(array('provider' => 'dynamic_routes'));

Does this mean we can't have a module named dynamic_routes?

Crell’s picture

alexpott: Yes. That's been the case for over a year now. It's unrelated to this issue.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
git ac https://drupal.org/files/issues/2102417-21.patch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 13205  100 13205    0     0   6724      0  0:00:01  0:00:01 --:--:--  9817
error: core/modules/system/lib/Drupal/system/Tests/Routing/RoutingFixtures.php: does not exist in index
dawehner’s picture

Status: Needs work » Needs review
FileSize
12.83 KB

Rerolled.

Crell’s picture

Status: Needs review » Reviewed & tested by the community

And back.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs reroll

Committed 8e729c3 and pushed to 8.x. Thanks!

dawehner’s picture

Thank you!

Status: Fixed » Closed (fixed)

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