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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

Status: Active » Needs review
Issue tags: +WSCCI
FileSize
455 bytes
pwolanin’s picture

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.

tim.plunkett’s picture

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

Crell’s picture

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?

dawehner’s picture

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.

Crell’s picture

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.

dawehner’s picture

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?

Damien Tournoud’s picture

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.

dawehner’s picture

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

dawehner’s picture

Status: Needs work » Needs review
FileSize
456 bytes

Let's use the proper pattern.

Crell’s picture

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.

alexpott’s picture

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.

Damien Tournoud’s picture

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.

pwolanin’s picture

@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?

dawehner’s picture

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

dawehner’s picture

FileSize
1.59 KB
1.59 KB

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

pwolanin’s picture

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?

dawehner’s picture

Damien Tournoud’s picture

@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.

dawehner’s picture

FileSize
1.75 KB

Rerolled after the revert.

dawehner’s picture

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.

dawehner’s picture

Status: Needs work » Needs review
FileSize
1.91 KB

This should work better.

dawehner’s picture

FileSize
1.32 KB

Missing interdiff.

Status: Needs review » Needs work

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

pwolanin’s picture

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

dawehner’s picture

Status: Needs work » Needs review
FileSize
2 KB

That is just a debug patch.

tstoeckler’s picture

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.

Crell’s picture

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

dawehner’s picture

Status: Needs work » Needs review
FileSize
2.74 KB

Another try.

Status: Needs review » Needs work

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

dawehner’s picture

Status: Needs work » Needs review
FileSize
1.24 KB
2.52 KB

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

pwolanin’s picture

how about using '' for the route name too?

dawehner’s picture

FileSize
1.09 KB
2.52 KB

Let's see whether this passes.

Crell’s picture

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.

dawehner’s picture

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

pwolanin’s picture

@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.

Crell’s picture

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.

Crell’s picture

Status: Needs review » Reviewed & tested by the community

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

catch’s picture

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.

catch’s picture

Issue summary: View changes

blub

dawehner’s picture

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

catch’s picture

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

Crell’s picture

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.

tstoeckler’s picture

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.

tstoeckler’s picture

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?

webchick’s picture

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?

dawehner’s picture

FileSize
13.46 KB
18.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.png
frontpage2.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.

dawehner’s picture

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

webchick’s picture

Status: Needs work » Reviewed & tested by the community

oh. Well then!

Crell’s picture

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

tstoeckler’s picture

+++ 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.

dawehner’s picture

FileSize
817 bytes
2.52 KB

That is simple, so let's do it.

catch’s picture

Status: Reviewed & tested by the community » Fixed

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

tstoeckler’s picture

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

Anonymous’s picture

Issue summary: View changes

updated issue summary