If you define a Route in a *.routing.yaml file like this the options don't make it to the Route object:

router_test_1:
  pattern: '/router_test/test1'
  defaults:
    _controller: '\Drupal\router_test\TestControllers::test1'
  requirements:
    _access: 'TRUE'
  options:
    foo: 'bar'

This is because Drupal\Core\Routing\RouteBuilder isn't passing the options to the Route's constructor.

Patch follows.

Follow-ups:
- #1859684: Expand routeBuilder unit test

CommentFileSizeAuthor
#14 1855204-routebuilder.patch876 byteseffulgentsia
#5 1855204-routebuilder.patch4.6 KBg.oechsler
#4 1855204-routebuilder.patch4.6 KBg.oechsler
#1 1855204-routebuilder.patch876 bytesg.oechsler
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

g.oechsler’s picture

g.oechsler CreditAttribution: g.oechsler commented
Status: Active » Needs review
FileSize
1855204-routebuilder.patch876 bytes
Crell’s picture

Crell CreditAttribution: Crell commented
Status: Needs review » Reviewed & tested by the community

I can't see how the bot would choke on this, and it's a legit bug fix. (The bot can disagree with me if it likes.)

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

Needs a test case.

g.oechsler’s picture

g.oechsler CreditAttribution: g.oechsler commented
Status: Needs work » Needs review
FileSize
1855204-routebuilder.patch4.6 KB

Here it is with a test for this.

g.oechsler’s picture

g.oechsler CreditAttribution: g.oechsler commented
FileSize
1855204-routebuilder.patch4.6 KB

s/self::/$this->/

Crell’s picture

Crell CreditAttribution: Crell commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

That's even better!

Lars Toomre’s picture

Lars Toomre CreditAttribution: Lars Toomre commented 
+++ b/core/lib/Drupal/Core/Routing/RouteBuilder.phpundefined
@@ -107,4 +105,13 @@ public function rebuild() {
+  /**
+   * Create a Route object from parsed yaml info.
+   */
+  public function getRouteFromInfo($route_info) {

Missing @param and @return directives in this docblock.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Needs work

Correct. Lars, can you please re-roll the patch with that added?

Also, it was my understanding we can't define multiple classes per file? What's up with that?

g.oechsler’s picture

g.oechsler CreditAttribution: g.oechsler commented
Status: Needs work » Needs review

What about http://drupal.org/coding-standards/docs#functions ?

Short form
Functions that are easily described in one line may use the function summary only [...]

Also I could not find a rule that says we can't define multiple classes per file. Could you please point me to it?

After thinking of this I come to the conclusion that a test for this is odd anyway.

Look what we want to do:

-            $route = new Route($route_info['pattern'], $defaults, $requirements);
+            $options = isset($route_info['options']) ? $route_info['options'] : array();
+            $route = new Route($route_info['pattern'], $defaults, $requirements, $options

Now #5 is a 4.6 KB patch. If we really need to split out the dummy classes (and certainly properly document all the noop methods) it will be even bigger. For me the "signal to noise ratio" of this would be below my personal feel-good threshold.

Anyway, the bug this is patching was introduced in #1801570 DX: Replace hook_route_info() with YAML files and RouteBuildEvent which came away without a test for RouteBuilder. So in facts it's not this issue that needs testing, it's #1801570.

I think #1 should be commited and we should file a follow up of #1801570 demanding a test case.

aspilicious’s picture

aspilicious CreditAttribution: aspilicious commented
Status: Needs review » Needs work

First of all it should be "CreateS". And we always describe params. It surprises me that line is in the docs manual.

About mutliple classes in one file. I'm surprised that even works. When you define multiple classes in a single file the autoloader can't load the classes. Maybe it works here because you're calling the classes from the same file. If some other tests want to use the same class they can't because there is no way it ever can load the class.

Last thingie, we are alwyas trying to replicate real drupal situations in tests. So why are you not using a config file and load it like we do in other tests?

Patch size isn't a problem.

g.oechsler’s picture

g.oechsler CreditAttribution: g.oechsler commented
Status: Needs work » Needs review

I think it is kind of funny we're discussing a method and some dummy classes for a test that wouldn't exist if the feature this patch is fixing had proper tests in the first place.

So as stated before what's missing is the tests for #1801570, which I believe is out of scope of this bugfix.

I still think #1 is a sufficient patch for the bug and needs review. Please ignore patch #5.

Crell’s picture

Crell CreditAttribution: Crell commented

Huh. I didn't even realize that the patch above was adding the extra test classes in the same file. That's my error.

We are following strict PSR-0 class loading, which does mean one class per file, period. The autoloader can't handle it otherwise.

g.oechsler: Welcome to core development. :-)

I'm personally comfortable with committing #1 as if. If we insist on a full on test suite, that can be a follow up unless someone pounces on this quickly, because that bug fix is a blocker for #1798214: Upcast request arguments/attributes to full objects. (Insert someone telling me "that's why we need unit tests" here.) #11's point is still valid.

effulgentsia’s picture

effulgentsia CreditAttribution: effulgentsia commented

I agree that if we commit anything here, it should be #1 and no more. But I don't understand why we need to commit this separately rather than as part of some use case that needs it, like #1798214: Upcast request arguments/attributes to full objects. When we get a use case that needs it in, the proper test for it would be a functional test of that use case in Drupal\system\Tests\Routing\RouterTest.

I'd like to point out that http://symfony.com/doc/2.0/book/routing.html contains no mention of what $options is for. http://symfony.com/legacy/doc/reference/1_4/en/10-Routing#chapter_10_sfo... documents it a little bit in relation to argument upcasting, but that's a legacy Symfony version. Is options useful for anything other than upcasting? If not, then I'm even more confused why this is a separate issue from #1798214: Upcast request arguments/attributes to full objects.

effulgentsia’s picture

effulgentsia CreditAttribution: effulgentsia commented
Status: Needs review » Reviewed & tested by the community
FileSize
1855204-routebuilder.patch876 bytes

Actually, I might have been too harsh in #13. Since we're using Symfony Route objects, and Symfony Route objects provide for a $options property, then even without any use case, Drupal's RouteBuilder should allow for it to be used. I do think we'll want functional tests for integrating it with argument upcasting, but that's a job for #1798214: Upcast request arguments/attributes to full objects. Separately, RouteBuilder unit tests would be nice, but what we need to unit test is the rebuild() method, not the getRouteFromInfo() method, which should either be inlined or protected. But, RouteBuilder already states that it can't be unit tested without #1331486: Move module_invoke_*() and friends to an Extensions class, so that shouldn't hold up this enhancement.

Therefore, reuploading #1 and RTBC'ing it.

Status: Reviewed & tested by the community » Needs work
Issue tags: -WSCCI

The last submitted patch, 1855204-routebuilder.patch, failed testing.

effulgentsia’s picture

effulgentsia CreditAttribution: effulgentsia commented
Status: Needs work » Needs review
Issue tags: +WSCCI

#14: 1855204-routebuilder.patch queued for re-testing.

effulgentsia’s picture

effulgentsia CreditAttribution: effulgentsia commented
Status: Needs review » Reviewed & tested by the community

And now that bot passed the retest, back to rtbc.

effulgentsia’s picture

effulgentsia CreditAttribution: effulgentsia commented

I opened #1859684: Expand routeBuilder unit test and added it as a follow-up to this issue's summary.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Fixed

I was going to knock this back to needs work for tests, but #14 explains why not, and that they'll be added elsewhere. Talking to Alex, he decided to create a major task to unit test this in a more holistic way (although it's currently postponed on #1331486: Move module_invoke_*() and friends to an Extensions class).

Committed and pushed to 8.x. Thanks!

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

Anonymous’s picture

(not verified) CreditAttribution: commented
Issue summary: View changes

Updated issue summary.