FileUrlGenerator::generate() does not work with externally hosted files using stream wrappers

Yes, one very awkward aspect of the refactor was that getExternalUrl() returns an external URL that we have to transform back, how about we add a check for UrlHelper::externalIsLocal().

This passes your test, only had a few minutes to throw it together, would be great if you could pick it up and deal with the dependency injection and BC for that and so on.

Ultimately, we want to replace getExternalUrl() with something that returns an Url object already, so we don't need to transform back and forth between URL object and string.

Meant to upload a full patch.

We might also want to add some cases with query arguments and so on to make sure that works as well.

As a bugfix, this is still something that we want to backport I think, possibly without DI to keep the potential for possible conflicts to a minimum, but that could be done in a 9.3 backport patch.

test fails in #7 look random.

There are some scenarios in which internal urls require to handle query parameters.

This patch propagate the $options in both cases.

I apologize, this is the good one. Previous patch missed a file.

Looking at the error from #7 onwards I see

The website encountered an unexpected error. Please try again later.
TypeError: Argument 1 passed to Drupal\Core\Routing\RequestContext::fromRequest() must be an instance of Symfony\Component\HttpFoundation\Request, null given, called in /var/www/html/web/core/lib/Drupal/Core/Routing/RequestContext.php on line 28 in Drupal\Core\Routing\RequestContext->fromRequest() (line 34 of core/lib/Drupal/Core/Routing/RequestContext.php). 

In my local lab.

Given it is still occurring and we did inject the request this may be a bit more than 'random'

It looks like that coming out of un-serilization from the page cache we don't have a request stack available to us.

Stacktrace:

Drupal\Core\Routing\RequestContext->fromRequest() (Line: 28)
Drupal\Core\Routing\RequestContext->fromRequestStack()
call_user_func_array() (Line: 279)
Drupal\Component\DependencyInjection\Container->createService() (Line: 176)
Drupal\Component\DependencyInjection\Container->get() (Line: 437)
Drupal\Component\DependencyInjection\Container->resolveServicesAndParameters() (Line: 240)
Drupal\Component\DependencyInjection\Container->createService() (Line: 176)
Drupal\Component\DependencyInjection\Container->get() (Line: 83)
Drupal\Core\Form\FormBase->__wakeup()
unserialize() (Line: 167)
Drupal\Core\Cache\DatabaseBackend->prepareItem() (Line: 122)
Drupal\Core\Cache\DatabaseBackend->getMultiple() (Line: 92)
Drupal\Core\Cache\DatabaseBackend->get() (Line: 306)
Drupal\page_cache\StackMiddleware\PageCache->get() (Line: 124)
Drupal\page_cache\StackMiddleware\PageCache->lookup() (Line: 82)
Drupal\page_cache\StackMiddleware\PageCache->handle() (Line: 48)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle() (Line: 51)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle() (Line: 23)
Stack\StackedHttpKernel->handle() (Line: 708)
Drupal\Core\DrupalKernel->handle() (Line: 19)

NOTE: D9.3 as I haven't yet setup a D9.4 lab so line numbers may be slightly off.

I've noticed that page cache unserialization thing through a form as well in some completely other context, that seems super weird to me that we end up with form object in internal page cache and something we should avoid as it makes page cache responses slower.

I'd suggest opening a separate issue for that, going back to the earlier patch that does not inject it with a @todo pointing to that other issue.

that seems super weird to me that we end up with form object in internal page cache and something we should avoid as it makes page cache responses slower.

It appears (in this case) we end up with the form in the cached due to needing to work with callback information.

s:4:"ajax";a:2:{s:14:"edit-css-frame";a:7:{s:8:"callback";a:2:{i:0;O:37:"Drupal\ckeditor_test\Form\AjaxCssForm":8:{s:15:" * requestStack";N;s:16:" * configFactory";N;s:13:" * routeMatch";N;s:14:" * _serviceIds";a:2:{s:16:"fileUrlGenerator";s:18:"file_url_generator";s:17:"stringTranslation";s:18:"string_translation";}

I can open an issue though I'm not really sure which component this should be filed against (form api, page_cache, or database cache backend?) and what to specify for a solution as I'm not sure what the alternatives for this could be.

Ah right, now I actually remember looking that up. And that again is part of the drupalSettings, right?

Because that makes absolutely no sense at all. We put all of #ajax into drupalSettings, but we should filter that out.

That's in \Drupal\Core\Render\Element\RenderElement::preRenderAjaxForm, we should remove that callback key in $settings. That could be a nice performance improvement on cached pages with ajax definitions.

That said, this won't guarantee that it can't happen in some other scenario, maybe in yet another issue we should make RequestContext more resilient when there is no request?

That said, this won't guarantee that it can't happen in some other scenario, maybe in yet another issue we should make RequestContext more resilient when there is no request?

This likely is a good idea. Your comment reminded me that working on the s3fs StreamWrapper I had another issue with a different middleware (banmiddleware) that lead to a null request stack even when called inside of a module #3211523: HTTP middleware implementations can result in the Request object being null in S3fsStream.php.

Having hit that issue in s3fs I am a bit concerned that this single test failure in core is an indicator that more problems will occur with the request stack as code continues to adopt the new service.

I wonder if for this particular issue if we should do

// @todo point to issue for request stack enhancement.
if (UrlHelper::externalIsLocal($external_url, $GLOBALS['base_url']))

as the the request_context, core StreamWrappers and s3fs module (not sure on flysystem) currently use this global value directly.

I can't point to any failures yet using \Drupal::service('router.request_context')->getCompleteBaseUrl()and I know using the global is not ideal/best practice however in this case I wonder if we are likely to hit more issues by trying to force the use of the request stack now before we look into making it more resilient? Maybe I'm overreacting to suggest it however I at least wanted to throw it out there as a possibly 'safer' option.

I am going to have to abstain from the drupalSettings question as it is outside my area of knowledge.

Passed a few URL's direct through the service and they appeared to be generating correctly to me.

I haven't been able to think of a scenario yet that I could cause my concerns from #18 with the request stack so since I can't present proof of a problem this looks RTBC to me.

Committed and pushed a60632076bf to 10.0.x and 7105aa278a8 to 9.4.x and 879b39ee5c5 to 9.3.x. Thanks!

this patch to fix external files urls