Problem/Motivation

In Drupal 7 generating a link to the frontpage was done by url(''). We need something similar with using the UrlGenerator coming from symfony.

Proposed resolution

The generate() method of the url generator just accepts the route name, so let's introduce a new pseudo-route to the frontpage, called 'front' which
can be used to provide a url to the frontpage.

This basically replaces the need for PathProcessorFront::processOutbound. On the other hand, we still need PathProcessorFront::processInbound, which then replaces the incoming frontpage with the actual configured frontpage path.

There is still some code in PathProcessorFront which deals with the frontpage, though this needs to be kept as long as url('') will be still supported.

Why is there no dynamic route created which has the pattern of the configured frontpage

The goal here is to be able to produce a link to the frontpage using the url generator, which needs a routename.
If you put the system.site.front variable as pattern, you would link to that page instead.

Files: 
CommentFileSizeAuthor
#54 front-2052995-54.patch2.52 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 58,012 pass(es).
[ View ]
#54 interdiff.txt817 bytesdawehner
#49 frontpage.png18.89 KBdawehner
#49 frontpage2.png13.46 KBdawehner
#36 front-2052995-36.patch2.52 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 57,735 pass(es).
[ View ]
#36 interdiff.txt1.09 KBdawehner
#34 routing-2052995-34.patch2.52 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 57,685 pass(es).
[ View ]
#34 interdiff.txt1.24 KBdawehner
#32 routing-2052995-32.patch2.74 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 57,677 pass(es), 13 fail(s), and 8 exception(s).
[ View ]
#28 drupal-2052995-28.patch2 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 57,974 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#25 interdiff.txt1.32 KBdawehner
#24 routing-2052995-24.patch1.91 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 57,870 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#21 drupal-2052995-20.patch1.75 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 57,849 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#17 drupal-2052995-17.patch1.59 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#17 interdiff.txt1.59 KBdawehner
#11 drupal-2052995-11.patch456 bytesdawehner
PASSED: [[SimpleTest]]: [MySQL] 57,810 pass(es).
[ View ]
#1 route-2052995-1.patch455 bytesdawehner
FAILED: [[SimpleTest]]: [MySQL] 57,304 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Comments

Status:Active» Needs review
Issue tags:+WSCCI
StatusFileSize
new455 bytes
FAILED: [[SimpleTest]]: [MySQL] 57,304 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

This doesn't really make sense to me - can we have a "fake" route like we had fake path ""?

then we should be able to pull the front page route out of config.

Status:Needs review» Needs work

The last submitted patch, route-2052995-1.patch, failed testing.

We have leading / on other routes, I guess this should be pattern: '/'

How does this relate to the fake < front > "path" we've always used for the front page? Does it replace it? Or should we be dynamically generating the front route based on the configured front page?

The goal here is to be able to produce a link to the frontpage using the url generator, which needs a routename.
If you put the system.site.front variable as pattern, you would link to that page instead.

This basically replaces the need for PathProcessorFront::processOutbound. On the other hand, we still need PathProcessorFront::processInbound, which then replaces the incoming frontpage with the actual configured frontpage path.

Ah, OK. Makes sense to me then. Although as Tim notes it would need to be '/'.

I'd rather make it a for-reals route rather than special case it as a "Fake route", simply because special casing is, 95% of the time, a code smell.

I'd rather make it a for-reals route rather than special case it as a "Fake route", simply because special casing is, 95% of the time, a code smell.

So your suggestion is to provide a dynamic route, which has all the settings coming from the route which is under the frontpage?

It doesn't make sense to me as well. The frontpage is part of an aliasing scheme, it belongs in the path processor system. Which means that yes, if you want to generate a URL to the frontpage, you need to go through several hops.

Can you please try to be a bit less opaque? It should be a simple task to provide a link to the frontpage right?

Status:Needs work» Needs review
StatusFileSize
new456 bytes
PASSED: [[SimpleTest]]: [MySQL] 57,810 pass(es).
[ View ]

Let's use the proper pattern.

Status:Needs review» Reviewed & tested by the community

Yes, that's what I meant. Fewer hoops, no magic. Just specify it somewhere and be done.

Status:Reviewed & tested by the community» Needs review

So if this:

basically replaces the need for PathProcessorFront::processOutbound.

Why are we not doing this here? Or should we be creating a followup?

Also should we be testing this new route?

Setting back to "needs review" to get the answers to the above questions.

I really don't like the idea of making a route that can never ever be matched, just for the sake of the path generation. This is confusing two completely different set of responsibilities: routing and path processing. The front page belongs in the second system. It should be completely unknown to the first.

@Damien - I agree this is problematic - I assumed we would be matching it and then delegating to the "real" route's page controller?

Otherwise, if I want to make a link to the "current" route based on the request I'd end up linking to some other page, not the front page?

basically replaces the need for PathProcessorFront::processOutbound.

Why are we not doing this here? Or should we be creating a followup?

We would have to use $urlgenerator->generate() in all places to remove the processOutbound. Outbound is still used in $urlgenerator->generateFromPath().

Also should we be testing this new route?

I Agreee

StatusFileSize
new1.59 KB
new1.59 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Here is a test, which tests both generate as well as generateFromPath, so we know both behave the same.

I think we cannot (yet) remove the outbound path processor since it is needed to handle the legacy system path , unless we want to just move the logic to the URLgenerator in generateFromPath() ? If you don't move it there should be a comment that it's legacy/deprecated.

Moving it the generateFromPath() might make it more clear that it's legacy?

@pwolanin:

I assumed we would be matching it and then delegating to the "real" route's page controller?

As far as I understand, this is not what's happening here. This route cannot ever be matched, because the front page is transformed by PathProcessorFront well before the matching happens.

We are only using this fake routes as a basis to generate URLs. And this feels just wrong.

Otherwise, if I want to make a link to the "current" route based on the request I'd end up linking to some other page, not the front page?

That's precisely why we have the outbound transformation. Both transformations need to exist, and they are perfectly symmetrical one another.

Let's please not muddy the waters between routing and path transformation.

StatusFileSize
new1.75 KB
FAILED: [[SimpleTest]]: [MySQL] 57,849 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Rerolled after the revert.

We are only using this fake routes as a basis to generate URLs. And this feels just wrong.

So would you be okay with hardcode the frontpage into the generate method and return '' in there?

Status:Needs review» Needs work

The last submitted patch, drupal-2052995-20.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.91 KB
FAILED: [[SimpleTest]]: [MySQL] 57,870 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

This should work better.

StatusFileSize
new1.32 KB

Missing interdiff.

Status:Needs review» Needs work

The last submitted patch, routing-2052995-24.patch, failed testing.

The fail seems to be related to running in a subdirectory, but I'm not able to readily reproduce it locally.

Status:Needs work» Needs review
StatusFileSize
new2 KB
FAILED: [[SimpleTest]]: [MySQL] 57,974 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

That is just a debug patch.

Otherwise, if I want to make a link to the "current" route based on the request I'd end up linking to some other page, not the front page?

That's precisely why we have the outbound transformation. Both transformations need to exist, and they are perfectly symmetrical one another.

I think there are separate use-cases here:
1. Link to a certain page that just happens to be the front page. We should link to the front page in this case, and that's what's the outbound path processor is for.
2. I want to link to the front page, but I don't care what is is. This is the reason that, in my opinion, a front page route makes sense. In this use-case the fact that the route is in fact dynamic and "magic" (although as you can see, there is not much magic going on in the patch) is just an implementation detail. From the perspective of linking to it, it's simply a route.

Status:Needs review» Needs work

The last submitted patch, drupal-2052995-28.patch, failed testing.

Yes, this is simply a cleaner way of linking to <front>

Status:Needs work» Needs review
StatusFileSize
new2.74 KB
FAILED: [[SimpleTest]]: [MySQL] 57,677 pass(es), 13 fail(s), and 8 exception(s).
[ View ]

Another try.

Status:Needs review» Needs work

The last submitted patch, routing-2052995-32.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.24 KB
new2.52 KB
PASSED: [[SimpleTest]]: [MySQL] 57,685 pass(es).
[ View ]

I think this line in the run-tests.sh should fix it properly, see Request::prepareBaseUrl().

how about using '' for the route name too?

StatusFileSize
new1.09 KB
new2.52 KB
PASSED: [[SimpleTest]]: [MySQL] 57,735 pass(es).
[ View ]

Let's see whether this passes.

I'm very skeptical about using arbitrary values for route names. Mostly because I don't know how far it's going to go before it breaks something. It also severely limits our ability to do string parsing with route names, eg, if we mention multiple route names in a string somewhere comma separated, that means commas can't be in the route name. (This came up in another issue, regarding permissions.) I'd prefer to keep the route names reasonably restricted for now. I really don't see what's wrong with linking to / meaning "the home page", since, er, that's what it is. It's far more self-documenting than a magic string is.

Edit: Forget that last sentence, totally off topic and dumb.

I seriously so much don't care, but I think 'front' is simpler than ''.

@Crell - linking to it how? We want to STOP using paths for internal links, right?

What if there is a language prefix or something else based on the current user's session?

I think the general approach here is right, and aligns very much with what's been used in Drupal before.

front vs isn't critical, thought I just think the latter would make a lot of documentation a little less obsolete.

Ignore my last sentence. I don't know what I was thinking and can't even blame it being late at night...

Still, I'd prefer to stick to alphanumeric-and-_. for route names for the other reasons mentioned. It gives us more flexibility. "front" is fine, as would be "home", or whatever.

Status:Needs review» Reviewed & tested by the community

RTBCing for the patch in comment #34, *not* 37.

Hmm this could be a follow-up but I'm also wondering a bit why we're not using a dynamic route here - we could clone whatever the 'real' front page route is for it or something? Not changing status but would be good to discuss a bit more, and it might need a comment as to why we're not doing that.

Issue summary:View changes

blub

@catch
This was answered in comment 6 #2052995-6: Add a route to the frontpage, and is now part of the issue summary.

@dawehner why can't you have a dynamic route with the pattern of '/' though?

catch: Why would you, when a static route is easier? Same end result with less code. We don't want any special casing. That's a code smell 95% of the time.

I didn't notice before that this was set to RTBC. While the simplicity is nice, I think the "_access: TRUE" is a problem. It is an edge case, but currently it is totally valid to set the front page to something that anonymous users cannot access. In that case links to the front page should not be generated.

In case I'm wrong at that does actually work already, we should add a test for that. Otherwise I think we should at least add a comment above the _access: TRUE that mentions this shortcoming.

To clarify:

* The current patch has the current problem: If the system.site.front config value is set to an access restricted route (e.g. '/admin') and an anonymous user is one a page which contains a link to the 'front' route. The link will be output. Clicking on this link will lead to a 403. A link to the route with the '/admin' path would not be output.

* I think this use-case is farfetched enough that we should not support it. If we were to go the route (no pun intended) of a dynamic route, as @catch suggests, this would not be a problem. I personally still find the simplicity of the current patch appealing enough that we should go for it.

* On the other hand, I also think we should be honest and document that this shortcoming, while far-fetched, exists. I proposed documenting that in the route above the '_access: TRUE' line, but @dawehner brought up in IRC that people will probably never find it there. So perhaps we should put that in a change notice? But which one?

Status:Reviewed & tested by the community» Needs work

I actually don't think that use case is very far-fetched. Consider the use case of an intranet. Committing this would represent a regression from D7 for those types of sites.

I too like the simplicity of this patch, though; can we simply make the access checker a bit smarter?

StatusFileSize
new13.46 KB
new18.89 KB

I am sorry but this is not a regression from Drupal 7. I just create a unpublished node, linked the frontpage to it "node/1" and created a menu link to <front>

According to your comment you would expect the menu link to disappear but it does not happen.
frontpage.pngfrontpage2.png

It is rather now the case that if you would like to you can achieve it in Drupal 8 in contrast to Drupal 7.

Just be be clear, the link appears even the user does not have access to it.

Status:Needs work» Reviewed & tested by the community

oh. Well then!

webchick: You can commit it. You haven't written code on it, and it's been more than 24 hours. ;-)

+++ b/core/modules/system/lib/Drupal/system/Tests/Routing/RouterTest.php
@@ -143,6 +144,24 @@ public function testControllerResolutionPage() {
+    // Setup the request context of the url generator. Note: Just calling the
+    // code without a proper request, does not setup the request context
+    // automatically.
+    $context = new RequestContext();
...
+    $this->container->get('url_generator')->setContext($context);

url -> URL

Also is this still true after #2062745: Automatically set the request context on the url generator?

Not downgrading, I'll open a follow-up if noone cares to re-roll before commit.

StatusFileSize
new817 bytes
new2.52 KB
PASSED: [[SimpleTest]]: [MySQL] 58,012 pass(es).
[ View ]

That is simple, so let's do it.

Status:Reviewed & tested by the community» Fixed

OK. Committed/pushed to 8.x, thanks!

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

Issue summary:View changes

updated issue summary