- Issue created by @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 thepage.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.
- π¬π§United Kingdom catch
Opened π RouteNormalizerRequestSubscriber redirects away from the front page when it's set to an alias Active too for the redirect issue.
- π¦πΊ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 ActiveOn 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.
- π©πͺ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. - π¨π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
- π¬π§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.
-
phenaproxima β
committed a92e22c6 on 1.x authored by
jurgenhaas β
Issue #3493615 by jurgenhaas, catch, pameeela, berdir, amateescu: Front...
-
phenaproxima β
committed a92e22c6 on 1.x authored by
jurgenhaas β
- πΊπΈUnited States phenaproxima Massachusetts
Merged into 1.x, thanks!
-
phenaproxima β
committed abc5fc59 on 1.0.x authored by
jurgenhaas β
Issue #3493615 by jurgenhaas, catch, pameeela, berdir, amateescu: Front...
-
phenaproxima β
committed abc5fc59 on 1.0.x authored by
jurgenhaas β
Automatically closed - issue fixed for 2 weeks with no activity.