Problem/Motivation

Symfony 5.2 introduces InputBag, which replaces ParameterBag in some places. The main change is that ::all() can take an argument:

https://github.com/symfony/http-foundation/blob/5.1/InputBag.php#L48-L51

Before:


$ajax_page_state = $request->request->all()['ajax_page_state'] ?? NULL;

After:
<?
$ajax_page_state = $request->request->all('ajax_page_state');
?>

Steps to reproduce

Proposed resolution

Subclass InputBag and replace $request on the $request object with our subclass, with a forward compatibility layer which will support both the old ParameterBag::all() as well as the new InputBag::all()

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

catch created an issue. See original summary.

longwave’s picture

Status: Active » Needs review
FileSize
2.12 KB

Very quick first pass at this.

longwave’s picture

FileSize
2.12 KB
andypost’s picture

  1. +++ b/core/lib/Drupal/Core/DrupalKernel.php
    @@ -589,6 +590,14 @@ public function preHandle(Request $request) {
    +    // Replace ParameterBags with InputBags for compatibility with Symfony 5.
    +    // @todo Remove this when Symfony 4 is no longer supported.
    

    needs a link to https://www.drupal.org/node/3198596

  2. +++ b/core/lib/Drupal/Core/Http/InputBag.php
    @@ -0,0 +1,34 @@
    + * Forward compatibility class for Symfony 5.
    + */
    +class InputBag extends ParameterBag {
    

    Suppose this class should have `@internal` to be removed in d10

alexpott’s picture

+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -589,6 +590,14 @@ public function preHandle(Request $request) {
+    if (!class_exists('Symfony\Component\HttpFoundation\InputBag')) {

I might be wrong but won't this entail a file scan on every request because we're checking for a class that does not exist?

longwave’s picture

Possibly, what should we do instead? Check the class name of the bags and rewrite them if they aren't InputBags already?

alexpott’s picture

@longwave yep I think that is a better option. It won't autoload.

catch’s picture

Was going to suggest not doing the check at all - that'd be fine for Drupal 9 and 10 since we know what we're dealing with, but it won't be fine for a force-upgrade of Drupal 9 to Symfony 5 and future changes to InputBag, which we also need to be able to do for core development and possibly contrib testing, so checking the classname seems best.

catch’s picture

Switched to instanceof, and added the internal from #4.2.

I marked the issue in #4.1 duplicate of #3162016: [Symfony 6] Retrieving a non-string value from "Symfony\Component\HttpFoundation\InputBag::get()" is deprecated so don't think we need an explicit @todo - that issue is already critical and blocking Drupal 10, and we won't be able to remove the @todo when it's fixed.

alexpott’s picture

Status: Needs review » Needs work
  1. +++ b/core/lib/Drupal/Core/Http/InputBag.php
    @@ -0,0 +1,37 @@
    +class InputBag extends ParameterBag {
    

    Let's make it final. Not reason not to. And add some test coverage.

  2. +++ b/core/lib/Drupal/Core/Http/InputBag.php
    @@ -0,0 +1,37 @@
    +      throw new BadRequestException(sprintf('Unexpected value for parameter "%s": expecting "array", got "%s".', $key, get_debug_type($value)));
    

    This exception doesn't exist as far as I can see. In SF5 the code is...

    
    /**
     * Raised when a user sends a malformed request.
     */
    class BadRequestException extends \UnexpectedValueException implements RequestExceptionInterface
    {
    }
    

    I think we should throw a \UnexpectedValueException here.

nikitagupta’s picture

Status: Needs work » Needs review
FileSize
2.63 KB
1.61 KB

Addressed #10.2, #10.1 is remaining.

Status: Needs review » Needs work

The last submitted patch, 11: 3198594-11.patch, failed testing. View results

catch’s picture

Status: Needs work » Needs review
FileSize
1.8 KB
3.52 KB

Added some test coverage.

longwave’s picture

Status: Needs review » Needs work

The new test needs an @group annotation.

catch’s picture

Status: Needs work » Needs review
FileSize
2.38 KB
3.54 KB

Whoops. Was going to make it legacy, but there's no deprecation message, so took that off leaving nothing. Sticking it in 'Http' should work, when we remove the class, the test itself will remind us to remove the test.

edit: excuse the crufty interdiff

longwave’s picture

+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -589,6 +591,14 @@ public function preHandle(Request $request) {
+      if (!($bag instanceof HFInputBag)) {

Bikeshedding here but I didn't immediately understand HFInputBag and I think aliasing it to SymfonyInputBag would be clearer.

Otherwise I think this is ready for RTBC but I don't think I can do it as I wrote the first versions of this.

catch’s picture

Don't think that's bikeshedding, it's an unnecessary abbreviation.

longwave’s picture

Status: Needs review » Reviewed & tested by the community

Discussed with catch, RTBC from me is OK as another core committer has to have final say on this anyway.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

As this will happen for every request I think we have to ask the performance question. I think we should consider adding the class to the classmap in core/composer.json to avoid the slower autoloading. I think it's all we can do.

+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -589,6 +591,14 @@ public function preHandle(Request $request) {
+      if (!($bag instanceof SymfonyInputBag)) {

I also wonder what's the effect of the autoload cache misses here. Like do we search 3 three times?

longwave’s picture

I also wonder what's the effect of the autoload cache misses here. Like do we search 3 three times?

You would think that negative results could be cached, is there a case where classes would suddenly appear at runtime that didn't exist before?

If we are concerned about this we could do if (get_class($bag) !== SymfonyInputBag::class)? InputBag is final so there can't be any subclasses that instanceof would pick up on but this check wouldn't.

alexpott’s picture

@longwave I don't think we allow negative results to be cached because we allow modules to be installed during a request and therefore a negative cache is v problematic.

catch’s picture

I think we should optimize the check to avoid autoloading rather than adding the class to the classmap.

catch’s picture

Status: Needs work » Needs review
Issue tags: +Needs manual testing

I just double-checked this by hacking the autoloader (with a file_put_contents($class) in loadClass()) - I am pretty sure instanceof does not trigger the autoloader anyway - i.e. it's just using the class name internally and comparing it to the actual class passed in. I can see our core InputBag autoloaded but not the Symfony one.

However would be good for someone else to confirm the same thing.

longwave’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs manual testing

Confirmed this with a similar hack: I added print $class; to loadClass() and then in DrupalKernel::boot() tried some experiments.

This does not print NonExistentClass:

    $var = $this instanceof NonExistentClass;
    exit;

Changing it to use new does print the class name:

    $var = new NonExistentClass;
    exit;

Therefore I am fairly confident the autoloader is not fired here.

However I have spotted that the list of classes loaded early does not quite match the preset classmap we have in composer.json, I will open a new issue about this.

alexpott’s picture

Status: Reviewed & tested by the community » Needs review

Ah yeah that makes sense about the instanceof check. Tested locally and you're right - I got confused with a class_exists() check. So then we have the other point... we're doing an additional autoload on every request handling page. So we should add InputBag to the classmap key in core/composer.json to avoid that work.... oh it's not every request... see below...

Also perhaps this should at the top of \Drupal\Core\DrupalKernel::handle() prior to booting the environment - otherwise middlewares won't be called with the SF5 compatibility layer. (And yes \Drupal\Core\DrupalKernel::handle() is called before \Drupal\Core\DrupalKernel::preHandle() ). The advantage of where it is called atm is that it does not affect page cached responses but I'm not 100% sure that that is correct.

Setting back to needs review for more discussion on the above.

longwave’s picture

Status: Needs review » Needs work

By definition, middleware directly handles Request objects so for consistency it would seem best to modify the Request object into the compatible state as early as possible - so ::handle() seems to be a good choice.

longwave’s picture

Moved the Request modification to ::handle() and added the new class to the classmap.

See also #3204539: Optimized classmap is out of date for a few other classes that are missing from the classmap.

Status: Needs review » Needs work

The last submitted patch, 27: 3198594-27.patch, failed testing. View results

longwave’s picture

Status: Needs work » Needs review
FileSize
6.1 KB
3.91 KB

Argh composer hashes.

catch’s picture

Status: Needs review » Reviewed & tested by the community

OK #29 looks solid to me, and I only changed test coverage so I think I'm OK to review the non-test code here. Tentatively back to RTBC. A bit concerned InputBag will end up as a zombie in the classmap after we remove it, but let's try to remember.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

I think we should add testing for zombie classmap entries in #3204539: Optimized classmap is out of date

Committed fc37ffa and pushed to 9.2.x. Thanks!

  • alexpott committed fc37ffa on 9.2.x
    Issue #3198594 by catch, longwave, nikitagupta, alexpott, andypost:...

Status: Fixed » Closed (fixed)

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