Updated: Comment #N

Problem/Motivation

RouteBuilder always iterates over the return from route_callbacks calling add() for each one to add it to the current RouteCollection. As a RouteCollection can also be returned from this, we can just call addCollection() instead in this case and be done with it. It seems more semantically correct from a code point of view to make this distinction.

Proposed resolution

Make those changes to RouteBuilder

Remaining tasks

Review, consensus.

User interface changes

None

API changes

None

CommentFileSizeAuthor
d8.routeBuilder-routeCollection-case.patch1.85 KBdamiankloip

Comments

dawehner’s picture

dawehner
Primary language German
commented
Status: Active » Reviewed & tested by the community

It is really a small think, though I like it.

webchick’s picture

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

Looks like we need tests for this?

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs tests

No, this works fine in HEAD because:
class RouteCollection implements \IteratorAggregate, \Countable {

You can still foreach over it.

As the OP says:

It seems more semantically correct from a code point of view to make this distinction.

That's why it's a minor task.

damiankloip’s picture

damiankloip commented

Nope, coverage was already added in #2145041: Allow dynamic routes to be defined via a callback. It covers using an array and a routeCollection in RouteBuilderTest anyway. Thank you past tim.plunkett.

EDIT: Oh Tim beat me to it, big surprise :P

webchick’s picture

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

Ok, got it, thanks.

Committed and pushed to 8.x.

Status: Fixed » Closed (fixed)

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