Follow-up to #2564921: In PHP-FPM environment, enabling a module in using a 'configure' route leads to an error page

Problem/Motivation

Drupal\Core\EventSubscriber\RouterRebuildSubscriber exists in core, but is not used.

See #2564921: In PHP-FPM environment, enabling a module in using a 'configure' route leads to an error page how it was discovered.
There is a work #2589967: Rebuild routes immediately when modules are installed to immediately rebuild router instead of scheduling it, so this EventListener will not be needed

Proposed resolutions

  • Remove the code
CommentFileSizeAuthor
#5 Core-RemovedUnneccesaryFile-2613400-0-8.0.x.patch1.61 KBzaurav

Comments

valthebald created an issue. See original summary.

valthebald’s picture

valthebald
Location Sofia
commented
Title: In PHP-FPM environment, enabling a module in using a 'configure' route leads to an error page » Remove Drupal\Core\EventSubscriber\RouterRebuildSubscriber
Category: Bug report » Task
Priority: Major » Normal
Status: Needs work » Active
valthebald’s picture

valthebald
Location Sofia
commented
Issue tags: -php-fpm +Novice
Related issues: +#2572293: Race condition triggerable by a single user due to router rebuild in kernel.terminate

Should be eligible for rc as followup of rc target #2572293: Race condition triggerable by a single user due to router rebuild in kernel.terminate

dawehner’s picture

dawehner
Primary language German
commented
Status: Active » Postponed

I would honestly postpne that until we have fixed anything in that area.

zaurav’s picture

zaurav
Primary language English
Location Oakton, VA
commented
StatusFileSize
new Core-RemovedUnneccesaryFile-2613400-0-8.0.x.patch1.61 KB

Removed core/lib/Drupal/Core/EventSubscriber/RouterRebuildSubscriber.php

valthebald’s picture

valthebald
Location Sofia
commented

@dawehner: but RouterRebuildSubscriber is not used, how its removal depends on other issues?

dawehner’s picture

dawehner
Primary language German
commented

@dawehner: but RouterRebuildSubscriber is not used, how its removal depends on other issues?

Well, it was intended to be there and work. Its just an implementation detail at the moment IMHO that not running it doesn't cause a bug.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Postponed » Reviewed & tested by the community

#356399: Optimize the route rebuilding process to rebuild on write added \Drupal\Core\Routing\RouteBuilder::destruct, so we definitely don't need this.

dawehner’s picture

dawehner
Primary language German
commented

The reason why I'm not comfortable with relying on ::destruct() is that it feels like we have less control over when the destructor is fired vs. the terminate event.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented

It's DestructableInterface, not __destruct. Not sure if that matters to you.

dawehner’s picture

dawehner
Primary language German
commented

OH I see, nevermind then!

  • catch committed 6722061 on 8.1.x 
    Issue #2613400 by zaurav, valthebald, dawehner, tim.plunkett: Remove...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.1.x, thanks!

While completely removing a class would normally be an API change, given this is an event subscriber the chance of someone extending it is as close to zero as it could possibly be.

Status: Fixed » Closed (fixed)

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

xjm’s picture

xjm
she/her
Primary language English
commented
Issue tags: -rc target triage