Front page always redirects to /home

Created on 12 December 2024, 2 months ago

Problem/Motivation

Found via πŸ“Œ Add performance testing Active because it started registering double of everything in the anonymous home page test. Every time you visit the the front page, you get 301 redirected to /home

This is an extra server round trip when people browse to or click on the front page of the site and seems unnecessary.

I didn't look into what's causing it, so it could be on purpose, or maybe a side-effect of redirect settings?

Either way it would be better from a performance point of view to avoid the redirect, especially for slower connections etc.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

πŸ“Œ Task
Status

Active

Component

Base Recipe

Created by

πŸ‡¬πŸ‡§United Kingdom catch

Live updates comments and jobs are added and updated live.
  • Performance

    It affects performance. It is often combined with the Needs profiling tag.

Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @catch
  • πŸ‡¬πŸ‡§United Kingdom catch
  • πŸ‡¦πŸ‡ΊAustralia pameeela

    It's definitely not on purpose, but seems like a bug caused by the order of things.

    We are setting the home page to '/home' because we can't assume the node ID of this page on install. But, I think the node doesn't exist when this config is set, so it's not connecting properly. If I simply save the site settings form, the redirect no longer occurs. (And if I change the initial config to be

    '/node/2'

    on install it also works as expected. But we can't do that.)

    This is a pretty weird exception because you can't save the form from the UI to set an invalid path, so this is pretty hard to replicate without recipes and default content. You can reproduce it if you manually import the config with an invalid path, and then create the node with the matching alias after -- but seemingly only with Pathauto installed; it doesn't occur in vanilla Drupal, I think because in that case you are setting the alias manually on the node.

    I haven't dug into this any deeper because I am sure there are others who would be much better equiped to look at how this is actually wired up in core :)

  • πŸ‡¬πŸ‡§United Kingdom catch

    OK I don't think I've ever seen this before. Changing to a bug report. Should be relatively easy to debug at least the symptom if not the root cause by sticking a backtrace in RedirectResponse and seeing what calls it.

  • πŸ‡¦πŸ‡ΊAustralia pameeela

    Not surprised you haven't seen it, before recipes it'd be nearly impossible to create this scenario without a great deal of effort. But now that I'm aware of it, it's really annoying me.

  • πŸ‡¦πŸ‡ΊAustralia pameeela

    Annoyed enough to actually look into it. So it's not recipe specific, and it's not because the page didn't exist when the config was set. You can reproduce it by setting page.front to any alias using drush, and you'll get the same redirect behaviour.

    The form validation swaps the alias with the path, so this isn't running if the config is changed outside of the form. Wondered why I never saw this before but I realised that we would always set page.front to a node path on a project, rather than an alias.

    I'm not going to go any deeper into the specifics of the redirect logic, but safe to say this is a core thing. Not sure whether to move this into the core queue or create a new issue for that, because I'd like to resolve this for Drupal CMS with a workaround in the meantime. I strongly suspect that ECA can come to the rescue here as a last resort.

  • πŸ‡©πŸ‡ͺGermany jurgenhaas Gottmadingen

    Right, if nothing else, here is what we could do with ECA:

    Listen to the config save event and if it's about system.site, move on: Read the page.front value and verify if it's an alias. If so, find the canonical path for it and override the value in the config.

    LMK if you want to have an ECA model for that.

  • πŸ‡¬πŸ‡§United Kingdom catch

    There might be at least three bugs here:

    1. Redirect module doesn't check for '/' before redirecting away from the front page, but if anything it should redirect /home back to the front page. It could also save all the lookup logic in RouteNormalizerRequestSubscriber if it short-circuited on '/'.

    2. PathMatcher::isFrontPage() only checks the configured string of the front page path, not the URL it resolves to etc. This might be 'by design'.

    3. Core should have a config listener that resolves aliases to system paths (where they exist) instead of the logic happening in the form.

    Both #1 and #3 seem like potentially quick fixes that could go into patch releases fwiw.

  • πŸ‡¦πŸ‡ΊAustralia pameeela

    Created πŸ› Updating page.front Active

  • πŸ‡¦πŸ‡ΊAustralia pameeela

    Ohhh I didn't realise that #1 meant that Redirect is actually causing this behaviour. So the core bug is only an indirect cause.

  • πŸ‡·πŸ‡΄Romania amateescu

    @catch, for #8.2, there's a very old core issue trying to change this behavior: πŸ› Aliased paths cannot be set as front page Needs review

    That issue now competes with πŸ› Updating page.front Active for the decision whether storing an alias in config is ok or not :)

  • πŸ‡·πŸ‡΄Romania amateescu

    Can't decide where to post this in the myriad of new issues opened, so I'll just do it here since it's related to #8.1.

    Redirect's RouteNormalizerRequestSubscriber has a much bigger problem, it completely disables core's alias preloading mechanism: πŸ› Feature or Bug? - The path-alias preloading is not used if the "Enforce clean and canonical URLs" option is enabled Active

    On the site where I discovered this (and had me digging through a lot of rabbit holes), we've disabled the Enforce clean and canonical URLs setting from the Redirect module, and went with this core change instead: #2641118-178: Route normalizer: Global Redirect in core β†’ (MR link)

  • πŸ‡¨πŸ‡­Switzerland berdir Switzerland

    > We are setting the home page to '/home' because we can't assume the node ID of this page on install. But, I think the node doesn't exist when this config is set, so it's not connecting properly. If I simply save the site settings form, the redirect no longer occurs. (And if I change the initial config to be '/node/2' on install it also works as expected. But we can't do that.)

    FWIW, the way we solve this in our project is an event subscriber on default content being created, and then check the UUID (which we can rely on) and set that as the frontpage. Not very nice, but works reasonable well.

  • πŸ‡©πŸ‡ͺGermany jurgenhaas Gottmadingen

    @berdir that's similar to the proposal in #7, which could be provided quickly without having to have another module just for the event subscriber. But in #8 it seems that this could be fixed in core instead, which of course would be much better if it could be done for the 1.0 release. If not, the ECA fallback could still be provided.

  • πŸ‡¨πŸ‡­Switzerland berdir Switzerland

    I'm not sure if #7 or πŸ› Updating page.front Active will help with this. The problem is execution order. default content is typically imported after config has been set by install profiles/modules/recipes. That means by the time the config is saved, /home isn't an alias *yet*, it will be created later.

    That's why you need to listen on content being created instead. Something like this:

      public function onContentImport(ImportEvent $event) {
        $created = $event->getImportedEntities();
        if (isset($created['837d4760-3e80-45f9-90bb-8183eae54b39'])) {
          $front = $created['837d4760-3e80-45f9-90bb-8183eae54b39'];
    
          \Drupal::configFactory()->getEditable('system.site')
            ->set('page.front', '/node/' . $front->id())
            ->save()
    

    This is trivial PHP code, but might not be so trivial to model it with ECA, not sure. And thinking about it, it's the default content module event, I don't think core added the events yet. You could also run it on node save, but this is a one-time thing and checking this for every node save is quit a bit of overhead.

  • πŸ‡¨πŸ‡­Switzerland berdir Switzerland

    I assumed that the default content is in the same recipe as the one that sets the config. It could be split of course, but AFAIK Drupal CMS is trying to avoid having too many small recipes.

  • Merge request !350Resolve #3493615 "Frontpage eca" β†’ (Merged) created by jurgenhaas
  • πŸ‡©πŸ‡ͺGermany jurgenhaas Gottmadingen

    Thanks @berdir for pointing that out, I missed that completely that the sequence of processing by recipes isn't helpful here. The overhead of handling this with ECA isn't much either, it's just 2 components, and it will only take action if the known uuid basic page gets inserted. To demonstrate that, I've provided that model as an MR. If we eventually end up with a different approach, it doesn't matter, it's been a simple fix that way.

  • Pipeline finished with Success
    about 2 months ago
    Total: 799s
    #381030
  • πŸ‡¨πŸ‡­Switzerland berdir Switzerland

    Yes, that makes sense as a hotfix at least.

  • πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

    I'm okay with hotfixing this with ECA, with a few provisos:

    • We need test coverage.
    • "Installer hotfix" is not the best name, let's change it to something more descriptive. Maybe "Front page redirect hotfix"?
    • Can the YAML file please get a long comment at the top of it which explains why it's needed, and has @see references to the relevant core and contrib issues?

    And another thought - since this is a hotfix and the PHP code to do it is trivial, would it be worth just doing this in the installer rather than with ECA? Presumably this would only affect new sites at install time.

  • πŸ‡¦πŸ‡ΊAustralia pameeela

    Presumably this would only affect new sites at install time

    I think we can/should assume this and only handle that case.

  • πŸ‡¬πŸ‡§United Kingdom catch

    fwiw there are at least two chicken and egg problems on πŸ› Updating page.front Active so it's not going anywhere fast. πŸ› Aliased paths cannot be set as front page Needs review is also hard.

    So working around this in the installer seems OK to me.

    Also seems not impossible that the default front page of Drupal CMS could change later - e.g. it could end up landing on project browser or a view or some kind of wizard or something, which wouldn't be entity routes at all.

  • πŸ‡¨πŸ‡­Switzerland berdir Switzerland

    > We need test coverage.

    That should be relatively straight forward. Visit the frontpage, make sure the current URL is still /.

    Can't comment on the part about install profile vs ECA, I don't know about the install order between recipes and the initial install profile what happens in which order. If the install profile installs a butch of recipes including the base on explicitly then yeah, it could afterwards load the node by UUID and set the home page instead of using a hook. That would certainly be more efficient than en ECA thing that runs every time a node is saved, forever (In theory, the ECA thing could so some fun stuff like delete itself after it has run to avoid that but that might be a bit weird).

  • πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

    Working around this in the installer would be acceptable, but I lean towards doing it with ECA for two reasons:

    • Only install-time stuff should be in the installer. This problem is more of an ongoing/runtime thing.
    • If someone installed the Drupal CMS Starter recipe without using our installer, they would not get this fix.
  • πŸ‡©πŸ‡ͺGermany jurgenhaas Gottmadingen

    Addressed to-do items from #21

    • Added the required test
    • Added a comment to the model
    • Renamed the model (label and ID)

    And another reason to be added to #25:

    With this ECA model, we don't have to maintain more code, we can now rely on ECA to still work with future minor and major Drupal core release.

  • Pipeline finished with Success
    about 2 months ago
    Total: 1097s
    #384643
  • πŸ‡¬πŸ‡§United Kingdom catch

    recipes/drupal_cms_starter/tests/src/FunctionalJavaScript/PerformanceTest.php can also be updated to visit '/' instead of '/home' - this will fail without the MR here because otherwise it records the queries used to generate the redirect, also fine to change that in a follow-up.

  • πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

    Perfect.

    I rephrased some parts of the comment in the pursuit of clarity, but sheeeit...this is a fine workaround, with documentation and test coverage. What else could I ask for? Nothing, I tells ya.

  • Pipeline finished with Skipped
    about 2 months ago
    #384908
  • πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

    Merged into 1.x, thanks!

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

Production build 0.71.5 2024