Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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
Comment | File | Size | Author |
---|---|---|---|
#29 | interdiff.3198594.17-29.txt | 3.91 KB | longwave |
#29 | 3198594-29.patch | 6.1 KB | longwave |
Comments
Comment #2
longwaveVery quick first pass at this.
Comment #3
longwaveComment #4
andypostneeds a link to https://www.drupal.org/node/3198596
Suppose this class should have `@internal` to be removed in d10
Comment #5
alexpottI 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?
Comment #6
longwavePossibly, what should we do instead? Check the class name of the bags and rewrite them if they aren't InputBags already?
Comment #7
alexpott@longwave yep I think that is a better option. It won't autoload.
Comment #8
catchWas 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.
Comment #9
catchSwitched 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.
Comment #10
alexpottLet's make it final. Not reason not to. And add some test coverage.
This exception doesn't exist as far as I can see. In SF5 the code is...
I think we should throw a \UnexpectedValueException here.
Comment #11
nikitagupta CreditAttribution: nikitagupta at Srijan | A Material+ Company for Drupal India Association commentedAddressed #10.2, #10.1 is remaining.
Comment #13
catchAdded some test coverage.
Comment #14
longwaveThe new test needs an @group annotation.
Comment #15
catchWhoops. 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
Comment #16
longwaveBikeshedding 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.
Comment #17
catchDon't think that's bikeshedding, it's an unnecessary abbreviation.
Comment #18
longwaveDiscussed with catch, RTBC from me is OK as another core committer has to have final say on this anyway.
Comment #19
alexpottAs 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.
I also wonder what's the effect of the autoload cache misses here. Like do we search 3 three times?
Comment #20
longwaveYou 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.Comment #21
alexpott@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.
Comment #22
catchI think we should optimize the check to avoid autoloading rather than adding the class to the classmap.
Comment #23
catchI 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.
Comment #24
longwaveConfirmed this with a similar hack: I added
print $class;
to loadClass() and then in DrupalKernel::boot() tried some experiments.This does not print NonExistentClass:
Changing it to use
new
does print the class name: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.
Comment #25
alexpottAh 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.
Comment #26
longwaveBy 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.
Comment #27
longwaveMoved 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.
Comment #29
longwaveArgh composer hashes.
Comment #30
catchOK #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.
Comment #31
alexpottI 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!