Problem/Motivation

After the fix in #3410022: Regression from #3295790 content-length header set earlier than expected, running database updates via drush updb throws an uncaught exception. In #3412160: Uncaught exception thrown when running database updates via drush we worked around this by removing final. This issue is for adding 

final<code> back.

The underlying issue is that lazy Symfony services do not support <code>final

classes, but this can be worked around with interface proxying:
https://symfony.com/doc/6.4/service_container/lazy_services.html#interface-proxifying

If there is more than one service tagged 'http_middleware' and at least one that is a responder, Drupal\Core\DependencyInjection\Compiler\StackedKernelPass::process() sets all those middleware services as lazy, other than the first responder.

The issue only seems to surface in Drupal (as observed so far) when the kernel's container is an instance of Symfony\Component\DependencyInjection\ContainerBuilder. The Drupal\Core\Update\UpdateKernel has its container set to an instance of ContainerBuilder, and the exception is thrown in the kernel terminate() method, when the call to ::getHttpKernel() leads to $this->container->get('http_kernel'), and as the decorated service is instantiated, the lazy middleware services are proxied, leading to an exception on final classes. Since this exception is thrown during the terminate phase, the exception is not displayed in browser when running updates via browser at update.php. The exception is logged, though. when running updates via drush, the exception stops the command from completing.

Proposed resolution

  1. Add back "final" to Drupal\big_pipe\StackMiddleware\ContentLength
  2. Add 'proxy' tags to http_middleware services set to lazy in Drupal\Core\DependencyInjection\Compiler\StackedKernelPass with the Symfony\Component\HttpKernel\HttpKernelInterface (and possibly Symfony\Component\HttpKernel\TerminableInterface) interfaces

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3412168

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

catch created an issue. See original summary.

godotislate’s picture

godotislate
he/him
commented

This might not be a drush issue? https://github.com/drush-ops/drush/issues/5848#issuecomment-1876021392

From this part of the exception thrown documented in #3412160: Uncaught exception thrown when running database updates via drush,

Fatal error: Uncaught Symfony\Component\VarExporter\Exception\LogicException: Cannot generate lazy proxy: class "Drupal\big_pipe\StackMiddleware\ContentLength" is final. in /var/
  www/html/vendor/symfony/var-exporter/ProxyHelper.php:92
  Stack trace:
  #0 /var/www/html/vendor/symfony/dependency-injection/LazyProxy/PhpDumper/LazyServiceDumper.php(136): Symfony\Component\VarExporter\ProxyHelper::generateLazyProxy(Object(Reflectio
  nClass), Array)
  #1 /var/www/html/vendor/symfony/dependency-injection/LazyProxy/Instantiator/LazyServiceInstantiator.php(33): Symfony\Component\DependencyInjection\LazyProxy\PhpDumper\LazyService
  Dumper->getProxyCode(Object(Symfony\Component\DependencyInjection\Definition), 'http_middleware...')
  #2 /var/www/html/vendor/symfony/dependency-injection/ContainerBuilder.php(1086): Symfony\Component\DependencyInjection\LazyProxy\Instantiator\LazyServiceInstantiator->instantiate

Looked at Symfony docs for Lazy Services: https://symfony.com/doc/6.4/service_container/lazy_services.html, and saw this:

Lazy services do not support final classes, but you can use Interface Proxifying to work around this limitation.

In PHP versions prior to 8.0 lazy services do not support parameters with default values for built-in PHP classes (e.g. PDO).

Suggestion there is to use interface proxying:

# config/services.yaml
services:
    App\Twig\AppExtension:
        lazy: 'Twig\Extension\ExtensionInterface'
        # or a complete definition:
        lazy: true
        tags:
            - { name: 'proxy', interface: 'Twig\Extension\ExtensionInterface' }

godotislate opened merge request !6017

godotislate’s picture

godotislate
he/him
commented

Opened MR 6017 as experiment to use interface proxying per previous comment in the service declaration. Ran drush updb with this change and saw saw no exceptions thrown and updates ran successfully.

Not sure how to reproduce the exception otherwise, so leaving it here for now.

godotislate’s picture

godotislate
he/him
commented
Issue summary: View changes
Status: Postponed » Needs review

Turns out it's not a Drush-specific problem, and the error was not obvious on update.php because the exception was thrown during kernel->terminate() call after page was already sent to browser. Exception does show up in the log.

Updated IS. Updated MR with proposed approach and added a unit test.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative

Ran the test-only job

1) Drupal\Tests\Core\DependencyInjection\Compiler\StackedKernelPassTest::testProcessWithStackedKernelAndFinalHttpMiddleware
Cannot generate lazy proxy for service "http_kernel.two".
/builds/issue/drupal-3412168/core/tests/Drupal/Tests/Core/DependencyInjection/Compiler/StackedKernelPassTest.php:151
/builds/issue/drupal-3412168/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
/builds/issue/drupal-3412168/vendor/phpunit/phpunit/src/Framework/TestSuite.php:684
/builds/issue/drupal-3412168/vendor/phpunit/phpunit/src/TextUI/TestRunner.php:651
/builds/issue/drupal-3412168/vendor/phpunit/phpunit/src/TextUI/Command.php:144
/builds/issue/drupal-3412168/vendor/phpunit/phpunit/src/TextUI/Command.php:97
FAILURES!
Tests: 3, Assertions: 15, Failures: 1.

Issue summary proposed solution match the fix.

Left a comment but that's just a curiosity and not a blocker if anyone see's that thread.

Change LGTM!

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Reviewed & tested by the community » Needs work

Tiny whitespace nitpick otherwise this looks good to me.

viren18febS made their first commit to this issue’s fork.

godotislate’s picture

godotislate
he/him
commented
Status: Needs work » Reviewed & tested by the community

Fixed whitespace. Moving back to RTBC since that was the only thing.

longwave’s picture

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

Decided not to backport as this isn't critical, if someone is doing something strange in 10.2 we won't break their code but they will have to fix for 10.3.

Committed c10494d and pushed to 11.x. Thanks!

  • longwave committed c10494df on 11.x 
    Issue #3412168 by godotislate, catch, smustgrave, longwave: Add back "...

Status: Fixed » Closed (fixed)

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

godotislate closed merge request !6017