AssetControllerBase validates the query string like this:
public function deliver(Request $request, string $file_name) {
...
// First validate that the request is valid enough to produce an asset group
// aggregate. The theme must be passed as a query parameter, since assets
// always depend on the current theme.
if (!$request->query->has('theme')) {
throw new BadRequestHttpException('The theme must be passed as a query argument');
}
if (!$request->query->has('delta') || !is_numeric($request->query->get('delta'))) {
throw new BadRequestHttpException('The numeric delta must be passed as a query argument');
}
if (!$request->query->has('language')) {
throw new BadRequestHttpException('The language must be passed as a query argument');
}
if (!$request->query->has('include')) {
throw new BadRequestHttpException('The libraries to include must be passed as a query argument');
}
Symfony 6.3 supports mapping query string parameters directly into controller arguments: https://symfony.com/blog/new-in-symfony-6-3-query-parameters-mapper
Allow #[MapQueryParameter] to be used in Drupal controllers.
Controllers can have query string values mapped directly to arguments.
Show commandsStart 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:
changes
Comments
Comment #2
longwaveAdded support for
MapQueryParametervia theQueryParameterValueResolverargument resolver, and convertedThemeControllerto use it, which removes all uses of theRequestobject and some validation code.Unsure if we need to provide BC in these controller methods though?
Comment #4
longwaveAdded a CR.
Comment #5
dcam commentedThis is pretty cool. I look forward to using it sometime.
The MR is pretty simple. It just has a lot of lines changed because those annoying, long conditions to check the parameter are removed and the contents are dedented. I installed the MR and the install, uninstall, and set as default routes continued to work. So this looks good to me.
Comment #6
alexpottSo this change results in something that was a 403 now being a 500... if you hit
admin/appearance/default?token=YOUR_TOKENi.e. without the theme query param. The new error is much more helpful than the 403...So that's good but this is a behaviour change we need to consider. I'm setting back to needs review to consider this.
Controllers are not part of BC - see https://www.drupal.org/about/core/policies/core-change-policies/bc-polic... - so I think we can make this change.
Also we need to file another bug report because doing
admin/appearance/default?theme&token=YOUR_TOKENalso produces a 500 but it really should report that the theme is missing. This is because \Drupal\system\Controller\ThemeController::willInstallExperimentalTheme() needs to handle the case where the passed in theme does not exist (it should just return FALSE) as all the calling functions have code to deal with this but it no longer works.Comment #7
alexpottFWIW I think a 500 is more correct than a 403.
Comment #8
longwaveGiven these are error conditions I was not that concerned about the change of response code, none of these cases should be happening in normal operation. The MR implies we don't have test coverage for any of this in core so I don't think external code or tests would care either.
I was slightly more concerned about the method signature change in case of subclasses or external callers but I also don't think it's worth providing BC here.
Maybe ThemeController was a bad example to convert? Not sure.
Tentatively setting back to RTBC as not sure what to change here.
Comment #9
alexpottOkay I agree the 403 to 500 is fine. I'll open the related issue to fix
\Drupal\system\Controller\ThemeController::willInstallExperimentalTheme().Committed and pushed 43c2d5ec907 to main and ee5a8e78b99 to 11.x. Thanks!
Comment #13
alexpottI created an issue to fix the missing theme issue I uncovered while testing this #3572785: \Drupal\system\Controller\ThemeController::willInstallExperimentalTheme() prevents code from reaching the theme was not found errors
Comment #14
longwaveThanks! Also opened #3572800: Use #[MapQueryParameter] to simplify query string handling to convert other uses in core; CSS/JS asset handling can likely be converted for example.