Problem/Motivation

Symfony CMF 1.3 got updated

Proposed resolution

Let's update it and drop some code Drupal had and got rewritten more generically in CMF

Remaining tasks

User interface changes

API changes

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

FileSize
11.16 KB
66.32 KB
Berdir’s picture

Status: Active » Needs review

I guess you wanted to set this to needs review?

The last submitted patch, 1: 2363537-core.patch, failed testing.

dawehner’s picture

Sure, ...

Crell’s picture

Status: Needs review » Needs work

5 files changed, 39 insertions, 263 deletions.

Yummy. :-)

+++ b/core/lib/Drupal/Core/Routing/RouteProvider.php
@@ -324,4 +326,28 @@ static function getSubscribedEvents() {
+  public function getRoutesPaged($offset, $length = NULL) {
+    $routes = $this->connection->select($this->tableName, 'router')
+      ->fields('router', ['name', 'route'])
+      ->range($offset, $length)
+      ->fetchAllKeyed();

According to PagedRouteProviderInterface, if $length is null then all routes should be returned. That means the range() method should only be called if($length).

(Thanks to znerol for catching this.)

Other than that, I think this looks fine.

catch’s picture

Priority: Major » Critical

Library update -> critical.

dawehner’s picture

Status: Needs work » Needs review
FileSize
67.83 KB
6.31 KB

Here is an update with some additional tests.

znerol’s picture

+++ b/core/vendor/composer/installed.json
@@ -2589,6 +2529,7 @@
+<<<<<<< ours

@@ -2611,16 +2552,57 @@
+>>>>>>> theirs
...
+<<<<<<< ours
...
+>>>>>>> theirs
...
+<<<<<<< ours

@@ -2645,6 +2627,21 @@
+>>>>>>> theirs

Oops.

mbrett5062’s picture

Please ignore this if I am being silly, and also if this is not the right time for nitpicks.

  1. +++ b/composer.lock
    @@ -1586,17 +1586,16 @@
    -            "target-dir": "Symfony/Cmf/Component/Routing",
    

    Not sure about this, but did you mean to remove "target-dir"?

  2. +++ b/core/vendor/symfony-cmf/routing/.travis.yml
    @@ -0,0 +1,31 @@
    +env: ¶
    

    Nitpick: Trailing whitespace.

aspilicious’s picture

Status: Needs review » Needs work
Crell’s picture

mbrett: composer.lock is a generated file, and any code in /core/vendor is 3rd party code, not ours. Both should be largely ignored.

dawehner’s picture

Status: Needs work » Needs review
FileSize
195.24 KB

+++ b/core/vendor/composer/installed.json

RAGE :) Well just remove it and run composer installagain ... and you should be done.

Having a recent composer version now forces the installed.json file to be written different ... so the patch size increases a lot now.

Crell’s picture

Status: Needs review » Reviewed & tested by the community

Changes to installed.json accounted for an extra 130 KB of patch? Apparently so. All the more reason that we need to stop checking vendor into Git. :-(

znerol’s picture

+++ b/core/lib/Drupal/Core/Routing/RouteProvider.php
@@ -324,4 +326,32 @@ static function getSubscribedEvents() {
+    if ($length) {

This still does not look 100% correct to me. If something passes 0 or "" (for whatever silly reason), then all routes will be loaded. Why not use isset($length) which conforms exactly to the interface.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

in IRC

09:07 alexpott
dawehner: do you have any thoughts on Znerol's comment?
09:08 dawehner
alexpott: he is right
dawehner’s picture

Status: Needs work » Needs review
FileSize
1.29 KB
195.37 KB

There we go

Crell’s picture

Status: Needs review » Reviewed & tested by the community

Either way.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Deleting all of that now-unnecessary code is lovely. I was concerned that doing so might break a crap ton of modules during beta, but the hunks in this patch seem to be limited to Composer, Symfony CMF, deleting said unnecessary code, and some additional test coverage. The one exception is core/lib/Drupal/Core/Routing/RouteProvider.php but this seems to be far enough under the covers that the disruption is minimal. Updating this library is also in service of resolving another core issue at #2203431: [meta] Various asset (JavaScript) libraries have to be updated to a (minified) stable release prior to 8.0.0, so I think we are good to go from the standpoint of https://www.drupal.org/contribute/core/beta-changes.

Committed and pushed to 8.0.x. Thanks!

  • webchick committed f0b86ce on 8.0.x
    Issue #2363537 by dawehner, znerol: Update CMF routing to 1.3 and remove...

Status: Fixed » Closed (fixed)

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