Convert file_create_url() & file_url_transform_relative() to service, deprecate it

If I am not mistaken,

git grep 'file_create_url(' | wc -l gives 274 instances of file_create_url in core code,

while

git grep 'file_create_url(' | git grep 'file_url_transform_relative(' | wc -l tells us that 228 of the 274 (83%) instances are actually part of a file_url_transform_relative(file_create_url(...)) construct.

For 8.1, does it warrant for a single method that would remove the need for double call?

Updates to #2.

Re #4, how about something like this?

Questions:

• Should we add an interface already or are there more functions which could be converted later?
• Do we need unit tests already for this issue? IMHO this would be a nice thing.

+++ b/core/includes/file.inc
@@ -193,57 +193,12 @@ function file_stream_wrapper_uri_normalize($uri) {
+ * @deprecated in Drupal 8.0.x-dev and will be removed before Drupal 9.0.0.

@@ -261,23 +216,12 @@ function file_create_url($uri) {
+ *
+ * @deprecated in Drupal 8.0.x-dev and will be removed before Drupal 9.0.0.
+ *   Use\Drupal\Core\File\FileUrlGenerator::transformRelative().

nitpick: IMHO this will be either 8.1 or 8.2

Still needs tests, since that's ostensibly why we're doing this. :-)

I don't see a linked contrib issue so I'm taking away the soft blocker tag.

1. +++ b/core/includes/file.inc
   @@ -193,57 +193,12 @@ function file_stream_wrapper_uri_normalize($uri) {
   + * @deprecated in Drupal 8.0.x-dev and will be removed before Drupal 9.0.0.
   

   8.3.x.

2. +++ b/core/includes/file.inc
   @@ -193,57 +193,12 @@ function file_stream_wrapper_uri_normalize($uri) {
   -      $path = $GLOBALS['base_url'] . '/' . UrlHelper::encodePath($options['path']);
   

   Soooo glad to get rid of $GLOBALS. :-)

3. +++ b/core/includes/file.inc
   @@ -261,23 +216,12 @@ function file_create_url($uri) {
   + * @deprecated in Drupal 8.0.x-dev and will be removed before Drupal 9.0.0.
   

   8.3.x.

Patch in #7 still applies to 8.4.x, but it also needs deprecation message updates for core version.

Updating target version in deprecation messages, for starters.

Two other functions look like good candidates for this service: file_valid_url(), file_build_uri().

+++ b/core/lib/Drupal/Core/File/FileUrlGenerator.php
@@ -0,0 +1,185 @@
+        // If this is not a properly formatted stream, then it is a shipped file.
...
+          $path .= '#' . $options['fragment'];

Line is exceeding 80 characters.
Applying the patch, please review.

Two main remarks:

1. I see this service exposes 2 public methods. So let's write a contract for them.
2. Searched the entire core for unit testing of file_create_url() but I failed to find. Correct me if I'm wrong. I think we need to add some testing along with the service.

Nits:

1. +++ b/core/includes/file.inc
   @@ -246,23 +201,12 @@ function file_create_url($uri) {
   + *   Use\Drupal\Core\File\FileUrlGenerator::transformRelative().
   

   Space after "Use". Aso, according to https://www.drupal.org/docs/develop/coding-standards/api-documentation-a...

   Lines containing comments (including docblocks) must wrap as close to 80 characters as possible without going over (...)

   So, "Use" can go to the first line.

2. +++ b/core/lib/Drupal/Core/File/FileUrlGenerator.php
   @@ -0,0 +1,186 @@
   +/**
   + * @file
   + * Contains \Drupal\Core\File\FileUrlGenerator.
   + */
   +
   

   Should be removed. See https://www.drupal.org/docs/develop/coding-standards/api-documentation-a...

   The @file doc block MUST be present for all PHP files, with one exception: files that contain a namespaced class/interface/trait, whose file name is the class name with a .php extension, and whose file path is closely related to the namespace (under PSR-4 or a similar standard), SHOULD NOT have a @file documentation block.

   This is the case.

3. +++ b/core/lib/Drupal/Core/File/FileUrlGenerator.php
   @@ -0,0 +1,186 @@
   +   *
   

   No empty line between @param blocks.

4. +++ b/core/lib/Drupal/Core/File/FileUrlGenerator.php
   @@ -0,0 +1,186 @@
   +   * @return string
   +   *   A string containing a URL that may be used to access the file.
   +   *   If the provided string already contains a preceding 'http', 'https', or
   +   *   '/', nothing is done and the same string is returned. If a stream wrapper
   +   *   could not be found to generate an external URL, then FALSE is returned.
   

   Rearrange text so lines are filled till the end. See my previous remark. Also, as I see , this method returns also FALSE. Need to reflect this in docs.

NW because of #20.

+++ b/core/lib/Drupal/Core/File/FileUrlGenerator.php
@@ -0,0 +1,180 @@
+   *   A string containing a URL that may be used to access the file.If the

Space before "If..."

#20.1, and partially 4 were not addressed.

Made changes according to comment #24

Changes as per comment #24 & also added interdiff.
#25 failed to apply on 8.4.x branch (giving error as "fatal: patch fragment without header at line 412: @@ -193,57 +193,12 @@ function file_stream_wrapper_uri_normalize($uri) {")

Any Update on this issue ?

Patch still applies. Re-running the test.

This will need a follow-up to remove usages, and a change record to talk about the deprecation.

reroll for 8.6

Proper patch, filed followup #2940025: Remove deprecated functions from file module

filed cr https://www.drupal.org/node/2940031

Now new class needs tests

+++ b/core/includes/file.inc
@@ -186,57 +186,12 @@ function file_stream_wrapper_uri_normalize($uri) {
+ * @deprecated in Drupal 8.6.x and will be removed before Drupal 9.0.0.
+ *   Use \Drupal\Core\File\FileUrlGenerator::generate().
...
+  return \Drupal::service('file_url_generator')->generate($uri);

@@ -254,23 +209,12 @@ function file_create_url($uri) {
+ * @deprecated in Drupal 8.6.x and will be removed before Drupal 9.0.0.
+ *   Use \Drupal\Core\File\FileUrlGenerator::transformRelative().
...
+  return \Drupal::service('file_url_generator')->transformRelative($file_url);

This functions needs link to change record and should trigger error https://www.drupal.org/core/deprecation#how-function

$ git grep file_url_transform_relative|wc -l
258

$ git grep file_create_url|wc -l
310

$ git grep -F 'file_url_transform_relative(file_create_url(' |wc -l
202

so most used both file_url_transform_relative(file_create_url($path . $string))
lots of time used in tests & wraps file entity url generation

So it means that the functions used mostly to build external URL and convert it to string

I think it needs rework

Patch
- a bit of clean-up logic
- docs fixes

+++ b/core/lib/Drupal/Core/File/FileUrlGenerator.php
@@ -0,0 +1,180 @@
+        return $uri;
...
+        return $path;
...
+      return $uri;
...
+        return $wrapper->getExternalUrl();
...
+        return FALSE;

cyclomatic complexity 10 and return type string or false

@Wim deprecation of the hook probably needs separate issue

If decorating a service would be the answer to everything then why does symfony have events? Service decorators can be useful in some cases, but they have problems, especially in drupal with our serialization handling. Events/hooks are still useful for anything that likely has multiple customizations and where the order might be relevant.

Converting to an event in a separate issue might be useful yes, but agreed that we shouldn't do that here.

It's always a nice thing when we can convert these old functions to properly testable, swappable services.

--- a/core/includes/file.inc
+++ b/core/includes/file.inc
@@ -186,57 +186,12 @@ function file_stream_wrapper_uri_normalize($uri) {
  *
  * @see https://www.drupal.org/node/515192
  * @see file_url_transform_relative()
+ *
+ * @deprecated in Drupal 8.6.x and will be removed before Drupal 9.0.0.
+ *   Use \Drupal\Core\File\FileUrlGenerator::generate().
  */
 function file_create_url($uri) {

 /**
@@ -254,23 +209,12 @@ function file_create_url($uri) {
  *   original value of $file_url.
  *
  * @see file_create_url()
+ *
+ * @deprecated in Drupal 8.6.x and will be removed before Drupal 9.0.0.
+ *   Use \Drupal\Core\File\FileUrlGenerator::transformRelative().
  */
 function file_url_transform_relative($file_url) {
-  // Unfortunately, we pretty much have to duplicate Symfony's
-  // Request::getHttpHost() method because Request::getPort() may return NULL

If we're deprecating these, we should have @trigger_error as well to enforce the deprecation.

@almaudoh to trigger error it need to replace usage and #37 explains that to replace usage we need to reconsider API for new service to prevent file_url_transform_relative(file_create_url( with is ~70% of usage

Notice that the patch need to be rerolled to incorporate the change from #2257231. That change also introduced some test cases.

Also I suggested fixing some edge case errors in #2983058.

Marking NW for the reroll.

Here's a re-roll of #37 including changes from #2257231. Posting here to run tests.

Remaining work:

1. Update tests in \Drupal\KernelTests\Core\File\UrlTransformRelativeTest::testFileUrlTransformRelative() to cover a new service.
2. What else?

I'm sorry but interdiff failed.

re-rolled

+++ b/core/lib/Drupal/Core/File/FileUrlGenerator.php
@@ -0,0 +1,175 @@
+    return preg_replace('|^https?://' . $http_host . '|', '', $file_url);

Move the original code here.
preg_replace('|^https?://' . preg_quote($http_host, '|') . '|', '', $file_url);

thnx @naveenvalecha , it was all about dots.
updated

for some reason i removed service. fixed.

1. +++ b/core/includes/file.inc
   @@ -184,57 +183,12 @@ function file_stream_wrapper_uri_normalize($uri) {
   + * @deprecated in Drupal 8.6.x and will be removed before Drupal 9.0.0.
   
   @@ -252,23 +206,12 @@ function file_create_url($uri) {
   + * @deprecated in Drupal 8.6.x and will be removed before Drupal 9.0.0.
   

   it should be 8.7.x now

2. +++ b/core/includes/file.inc
   @@ -184,57 +183,12 @@ function file_stream_wrapper_uri_normalize($uri) {
   +  return \Drupal::service('file_url_generator')->generate($uri);
   
   @@ -252,23 +206,12 @@ function file_create_url($uri) {
   +  return \Drupal::service('file_url_generator')->transformRelative($file_url);
   

   Also it needs to throw error, test deprecated code & remove usage according to https://www.drupal.org/core/deprecation#how-function

CR updated to mention both functions

updated

Not sure it makes sense to trigger error now because usage of the functions are very broad

core8$ git grep file_create_url |wc -l
314
core8$ git grep file_url_transform_relative| wc -l
261

But the issue gives me 403 for some reason #2940025: Remove deprecated functions from file module

Patch updated to not trigger error.
After researching core code we can see a lot of internal usage of this two functions. So Issue need to be spitted to another one, where we can make a decision of usage this functions/methods in every case, like if its related to file entity, make sense to add method to drupal/core/modules/file/src/Entity/File.php and other cases need to be re-thinked aswell. But imho its out of scope of this task

1. +++ b/core/includes/file.inc
   @@ -184,57 +183,14 @@ function file_stream_wrapper_uri_normalize($uri) {
     * @see https://www.drupal.org/node/515192
     * @see file_url_transform_relative()
   + *
   + * @deprecated in Drupal 8.7.x and will be removed before Drupal 9.0.0.
   + *   Use \Drupal\Core\File\FileUrlGenerator::generate().
   + *
   ...
     */
    function file_create_url($uri) {
   

   This is tricky because we're trying to avoid adding more @deprecated without a @trigger_error(), at the same time, 300 usages is obviously too much to update in a single patch.

   We should probably do at least a few of them, though and we need a plan to do the others, which means I guess a follow-up (meta) issue to organize that.

   I know some people won't like it, but with 300+ usages and a lot of that in tests and other places that we can't/won't actually inject, I'm wondering if this one would deserve a method on \Drupal: \Drupal::fileUrlGenerator()->generate()?

   And last, the change record should also cover the $relative argument and show how to convert the very common combined usage of file_url_transform_relative(file_create_url().

2. +++ b/core/lib/Drupal/Core/File/FileUrlGenerator.php
   @@ -0,0 +1,174 @@
   +/**
   + * Generates file URLs.
   + */
   +class FileUrlGenerator {
   

   Not sure if this needs an interface or not. It's certainly not a value object or anything like that, so possibly?

3. +++ b/core/lib/Drupal/Core/File/FileUrlGenerator.php
   @@ -0,0 +1,174 @@
   +   * @param bool $relative
   +   *   Set to TRUE to return a URL that is relative to the current request's
   +   *   base URI.
   +   *
   

   I guess the question now is what the default value for this should be.

   Consdering that relative should be the default now, I'd say we should do $relative = TRUE?

4. +++ b/core/lib/Drupal/Core/File/FileUrlGenerator.php
   @@ -0,0 +1,174 @@
   +   *
   +   * @see https://www.drupal.org/node/515192
   +   * @see file_url_transform_relative()
   

   that d.o node is the D7 documentation, I think we can just remove that.

   and the reference to the deprecated function is also not needed and the replacement is on the same service and easy to find.

@Berdir
1. Agree, imho we can create new issue for replacement deprecated usage. But i guess it can impact on contrib modules a lot. Still i'd like to have new issue for replacement.
2. Make sense
3. It depends from usage, from what i saw in core code, very often usage is "file_url_transform_relative(file_create_url()" - so agree #3
4. Agree

From #2244513-46: Move the unmanaged file APIs to the file_system service (file.inc), which suggested to move all the following methods on a service:

file_uri_target()
file_stream_wrapper_uri_normalize()
file_create_url() ?
file_url_transform_relative() ?
file_valid_uri()
file_build_uri()

It's easier to add them at all once instead of having to extend an interface later on.

Re #68:

1. not sure what you mean with contrib impact, it will have that anyway, if anything, adding a \Drupal method will make it easier. Maybe skip calls in tests and convert all others here?
3. Note that I'm adding $file->createFileUrl() in #2402533: Provide File::createFileUrl() as a replacement for the deprecated File:url() implementation, with a $relative = TRUE argument, so making this consistent would be great.

Hm, on closer investigation, I don't think any of these additional methods belong into this new service.

* file_uri_target() is the counter-part to file_uri_scheme(), which is already on file_system
* file_stream_wrapper_uri_normalize() is also very related to these, they're all about (stream wrapper) URI's, not file URLs.
* file_valid_uri() is again about stream wrapper URI's and as a small wrapper around two functions/methods that were already moved to file_system.
* file_build_uri() also is about creating stream wrapper URIs and is tightly connected to these other functions.

FileSystem is getting quite big with that other issue, so maybe these methods could be on the stream wrapper manager, but then we'd also need to move some existing methods from FileSystem to that to avoid a cross-dependency.

I did however start with updating all the deprecated calls and it's actually not too bad. Most of them are in tests and a *ton* of them were in a few unit tests that I've now properly mocked and completely replaced with the expected hardcoded string instead of calling a mock function for the expected URLs.

I also added an interface for this, I can imagine this being something that sites might want to have different implementations of, instead of using the file_url_alter() hook for example which has some limitations on what it can do.

Most of the remaining calls can probably be done with a global search & replace, but I stopped for now as there are a bunch of overlaps with #2646744: \Drupal\Core\Url does not accept root-relative (file) URLs, making it impossible to let LinkGenerator create root-relative file URL links and it will be easier to convert once that is in, especially since that will use $file->createFileUrl() in many places.

+++ b/core/lib/Drupal/Core/File/FileUrlGeneratorInterface.php
@@ -0,0 +1,59 @@
+/**
+ * Created by PhpStorm.
+ * User: berdir
...
+ * Time: 16:27
+ */

note for the next reroll, remove this ;)

Oops, this should be better.

Fixed the inconsistent relative argument.

Fixed the twig extension test and added explicit unit test coverage for file_url(), now that we can mock it.

Also noticed that $relative was not considered for all cases, actually only the one that didn't really matter that much (in case you passed in an existing absolute URL, which was most likely external), so that caused some other interesting test fails, and we only start to notice now as we actually start to use that option.

Still applied with git apply -3.

Test coverage looks good so removing the tag.

1. +++ b/core/lib/Drupal/Core/Asset/JsCollectionRenderer.php
   @@ -17,14 +18,24 @@ class JsCollectionRenderer implements AssetCollectionRendererInterface {
   +   * @param \Drupal\Core\File\FileUrlGeneratorInterface $file_url_generator
   +   *   The file URL generator.
   
   +++ b/core/lib/Drupal/Core/Template/TwigExtension.php
   @@ -63,13 +71,16 @@ class TwigExtension extends \Twig_Extension {
   +   * @param \Drupal\Core\File\FileUrlGeneratorInterface $file_url_generator
   +   *   The file URL generator.
   
   +++ b/core/modules/ckeditor/src/Plugin/Editor/CKEditor.php
   @@ -74,13 +82,16 @@ class CKEditor extends EditorBase implements ContainerFactoryPluginInterface {
   +   * @param \Drupal\Core\File\FileUrlGeneratorInterface $file_url_generator
   +   *   The file URL generator.
   

   Do you want to mention that is optional now and will be required in 9.0.0?

2. +++ b/core/modules/color/color.module
   @@ -487,7 +489,7 @@ function color_scheme_form_submit($form, FormStateInterface $form_state) {
   -      $css_optimizer = new CssOptimizer();
   +      $css_optimizer = \Drupal::service('asset.css.optimizer');
   

   Should be mention this in the change record?

Fixes for #82

1. I added the trigger_error dance in these constructors
2. Also added a trigger error here, and moved calling the service out of the nested for loops so we only do it once

Did a few code sniffer cleanups too.

I also updated the change record for #82.2

Although the IS says we'd do the remove usages in a separate issue, I went ahead and added @trigger_error and replaced usages in this issue. It seems to be the preferred way to do it all in one go.

I also updated deprecation messages to reference 8.8.0.

Replaced a few missed instances of file_url_transform_relative()

Not sure why relative paths aren't being returned for a lot of these fails.

Instead of passing flags that change method behaviour, I usually prefer to add a method, e.g.:

<?php

  public function generateRelative($uri) {
    return $this->generate($uri, TRUE);
  }

?>

Thoughts?

Wow. I really misunderstood the API. I've hopefully corrected most of the mis-use of transform-relative in this patch.

How about:

<?php
public function generateAbsolute($uri);
public function generateRelative($uri);
public function transformRelative($uri);
?>

I'm open to improve the API and make those methods more explicit, but it doesn't solve the underlying problem from #2646744: \Drupal\Core\Url does not accept root-relative (file) URLs, making it impossible to let LinkGenerator create root-relative file URL links, that we are unable to create a Url object from a relative-transformed URL.

I'm thinking about being able to return a URL object from those methods, or have a method that does that, and then internally do the relative-transform in a way that includes removing the base path already, because then we can be sure that it does contain it. Not fully thought through yet...

Yeah, I don't think I have my head around it either. I will be at DrupalCon Seattle if anyone wants to discuss?

Fixes for #92

Looking into better methods and returning Url objects...

This adds 3 methods instead of just one with a boolean argument:

generate()
generateAbsolute()
generateUrl(), which returns a Url object.

The 3 methods are currently completely separate on purpose, because there are many difference cases, and instead of having to generate a full URL only to strip it off again, we can now in most cases just generate what we need. And we can also create correct Url objects that don't have to be root-relative, which means most of what #2646744: \Drupal\Core\Url does not accept root-relative (file) URLs, making it impossible to let LinkGenerator create root-relative file URL links was doing is now basically included here.

Also updated a lot of tests to use $file->createFileUrl() (which still uses a boolean flag, can't change that anymore, but it's still much simpler)

Known issues:
* Url doesn't support data: urls but file_create_url() does
* image styles are still absolute, will need to figure out if we can fix that here, because we are currently removing the cache context, but it was incorectly set anyway.

Very much WIP, just want to see where this is at in regards to test fails. This is unfortunately much more than just 1:1 conversion now, we *could* try to split it up, but having and using those extra methods verifies that it works, so we'll see.

Will also need to carefully review the existing test coverage to make sure that actually makes sense now and I plan to write a comprehensive unit test for FileUrlGenerator that tests the many possible variations for all 3 methods.

Another thing I noticed again in kernel tests is how messed up their public/base path is: 'http://localhost/vfs://root/sites/simpletest/29803809/files/file.png', we should try to make that a bit saner, but not here.

Fixing tests and cleanup. Definitely confused about the image style situation, most tests assert for a relative URL, but that works on an absolute one too and there doesn't seem to be any tests that assert that it actually is absolute. Wanted to but not changed that, seems like that's better for a dedicated issue.

The url.site assertion is also a bit pointless in \Drupal\Tests\image\Functional\ImageFieldDisplayTest::_testImageFieldFormatters() because that's still TRUE as NodeController always adds that.

A couple of minor nits:

1. +++ b/core/lib/Drupal/Core/Asset/CssOptimizer.php
   @@ -16,6 +17,27 @@ class CssOptimizer implements AssetOptimizerInterface {
   +      @trigger_error('Calling CssOptimizer::__construct() with the $file_url_generator argument is supported in drupal 8.7.0 and will be required before 9.0.0. See https://www.drupal.org/node/2940031.', E_USER_DEPRECATED);
   

   Needs to be 8.8.0 now

2. +++ b/core/lib/Drupal/Core/Asset/JsCollectionRenderer.php
   @@ -17,14 +18,28 @@ class JsCollectionRenderer implements AssetCollectionRendererInterface {
   +      @trigger_error('Calling JsCollectionRenderer::__construct() with the $file_url_generator argument is supported in drupal 8.7.0 and will be required before 9.0.0. See https://www.drupal.org/node/2940031.', E_USER_DEPRECATED);
   

   ditto

Hi guys, I was brought to this issue due to having an issue with URLs in my project that aren't encoded. In my project, I use the
tag where I have a tag that looks like this:
<source srcset="//test.domain.com/sites/default/files/styles/my_image_style/public/This is a test image.jpg 1x" media="all and (min-width: 992px)" type="image/jpeg">
Before I used the CDN module, the tag looked like this:
<source srcset="//test.domain.com/sites/default/files/styles/my_image_style/public/This%20is%20a%20test%20image.jpg 1x" media="all and (min-width: 992px)" type="image/jpeg">

The URL without URL encoding wasn't able to reach the image because it only saw the valid link to be "//test.domain.com/sites/default/files/styles/my_image_style/public/This" because after was already a space and treated everything after it to be another value like the "1x".

Since this issue is connected to the `file_create_url($uri)` function, this function calls `drupal_encode_path($uri)` then passes the URI or path to the `rawurlencode()` PHP function that adds converts spaces into "%20". With this finding, was it intended to forego URL encoding inside the `FileUrlGenerator::generate()` method or it needs to be there?

------
UPDATE
My apologies for wrongly posting this comment. This was an issue I found referenced in the CDN D8 module. Please disregard.

> My apologies for wrongly posting this comment. This was an issue I found referenced in the CDN D8 module. Please disregard.

No worries.

But yes, the goal of this is to mostly just refactor how things work, with minimal functional changes. I was also thinking that this sounds like a bug in the CDN module instead, if it does return absolute URL's then it need to take care of encoding, just like our own code here already does. We can't know if it's already encoded or not, so might end up encoding it twice.

Ok, figured out the deal with image styles. They aren't absolute on HEAD, but the trick is that buildUrl() *does* return an absolute URL and that is set in template_preprocess_image_style(), but then template_preprocess_image() makes it relative again, and that didn't work yet. So we're calling generate()/file_create_url() repeatedly on the same thing, that's not confusing at all.

The bug was that generate() didn't call transformRelative() on http/https URL's, that's fixed now and should be good enough for this issue.

Hm, something ate the actual patch.

Looking good. Just a few comments:

1. +++ b/core/lib/Drupal/Core/Asset/CssOptimizer.php
   @@ -257,7 +279,23 @@ public function rewriteFileURI($matches) {
   +      @trigger_error('The file_generator service must be injected into CssOptimizer.');
   

   Do we need to specify the type of error here? Also, do we want to point to the change record?

2. +++ b/core/lib/Drupal/Core/Asset/JsCollectionRenderer.php
   @@ -17,14 +18,28 @@ class JsCollectionRenderer implements AssetCollectionRendererInterface {
   +      @trigger_error('Calling JsCollectionRenderer::__construct() with the $file_url_generator argument is supported in drupal 8.7.0 and will be required before 9.0.0. See https://www.drupal.org/node/2940031.', E_USER_DEPRECATED);
   

   Needs to be updated to 8.8.0 and follow standard message format.

3. +++ b/core/lib/Drupal/Core/File/FileUrlGenerator.php
   @@ -0,0 +1,237 @@
   +  public function generateUrl($uri) {
   ...
   +  public function generate($uri) {
   ...
   +  public function generateAbsolute($uri) {
   

   Is there no way to share common logic here? Seems to be a bit of duplicated code.

4. +++ b/core/lib/Drupal/Core/File/FileUrlGeneratorInterface.php
   @@ -0,0 +1,88 @@
   +  public function generate($uri);
   ...
   +  public function generateUrl($uri);
   

   I think we need clearer documentation on the difference between these two methods. Perhaps its the method names themselves? I'd be interested to hear others opinions on the method names.

5. +++ b/core/lib/Drupal/Core/Template/TwigExtension.php
   @@ -63,13 +71,20 @@ class TwigExtension extends \Twig_Extension {
   +      @trigger_error('Calling TwigExtension::__construct() with the $file_url_generator argument is supported in drupal 8.7.0 and will be required before 9.0.0. See https://www.drupal.org/node/2940031.', E_USER_DEPRECATED);
   

   Needs to be 8.8.0.

6. +++ b/core/modules/color/color.module
   @@ -491,7 +493,7 @@ function color_scheme_form_submit($form, FormStateInterface $form_state) {
   +      $css_optimizer = new CssOptimizer(\Drupal::service('file_url_generator'));
   

   Should we create a variable for this outside the foreach?

7. +++ b/core/modules/file/file.module
   @@ -1452,13 +1453,9 @@ function template_preprocess_file_link(&$variables) {
   -  $variables['#cache']['contexts'][] = 'url.site';
   

   Do we need to add something about the removal of the cache context to the change record?

Thanks for the review. Yes, the deprecation messages definitely need work, didn't bother with that yet.

3. I avoided that for now on purpose. Each method has different cases, some convert from relative to absolute, others from absolute to relative. And there are 4 or so different return cases. The only way to more than 2-3 lines of code between the thre would be to do stuff twice. There's still the option to only have generateUrl(), as WimLeers suggested, but looking at the usage, the majority of calls do not need Url objects, so again, going through that would be extra work.

4. i tried to document the different methods and differences on the class itself, felt like that was fairly clear, but open to suggestions on how to improve it.

6. It is my understand that this is done on purpose, which is also why it doesn't use the service. Each loop sets properties on that object, I don't know if it would share some state if reused. An earlier patch actually just used the service and it didn't cause any test fails.. but it's color.module, I can't imagine our test coverage there is very good.

7. Yeah maybe, still an option to not make all those absolute => relative changes in this issue, but as I mentioned above, then we don't have any real-world usage of the new method.. we could do generateUrl()->setAbsolute() I suppose, but then we're again back to the topic of transforming it to relative only to make it absolute again.

4. Maybe generate() can be generatePath() as it always returns a relative path?

Fixed the deprecation messages and some code sniff issues to keep the ball rolling.

4. Maybe generate() can be generatePath() as it always returns a relative path?

I was going to suggest generateRelative() too, but then read the docblock and it says this can be altered by CDN etc. to return an absolute URL. So I think this is ok for now.

Re-roll of #112

Fix usage of deprecated functions in recent commits.

Wondering if this fail is the due to the changes in #102?

Looks like it. Here's a fix for MediaEmbedFilterTest.

Here is a walk by review. A lot of points are related to DIC. If those parts are deliberately left for now then please ignore parts of the review.

1. +++ b/core/modules/file/file.module
   @@ -1504,7 +1502,7 @@ function template_preprocess_file_link(&$variables) {
   -  $variables['link'] = \Drupal::l($link_text, Url::fromUri($url, $options));
   +  $variables['link'] = Link::fromTextAndUrl($link_text, $url->setOptions($options));
   

   Unrelated change.

2. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/BaseFieldFileFormatterBase.php
   @@ -47,9 +46,7 @@ public function viewElements(FieldItemListInterface $items, $langcode) {
   +      $url = \Drupal::service('file_url_generator')->generateUrl($items->getEntity()->getFileUri());
   

   The service should be injected.

3. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileUriFormatter.php
   @@ -51,10 +51,7 @@ public function settingsForm(array $form, FormStateInterface $form_state) {
   +      $value = \Drupal::service('file_url_generator')->generate($value);
   

   The service should be injected.

4. +++ b/core/modules/file/src/Plugin/views/field/File.php
   @@ -64,13 +64,7 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
   +      $this->options['alter']['url'] = \Drupal::service('file_url_generator')->generateUrl($this->getValue($values, 'uri'));
   

   The service should be injected.

5. +++ b/core/modules/image/src/Plugin/Field/FieldFormatter/ImageFormatter.php
   @@ -198,18 +198,12 @@ public function viewElements(FieldItemListInterface $items, $langcode) {
   +    $file_url_generator = \Drupal::service('file_url_generator');
   

   The service should be injected.

6. +++ b/core/modules/media/src/Plugin/Filter/MediaEmbed.php
   @@ -214,7 +214,7 @@ protected function renderMedia(MediaInterface $media, $view_mode, $langcode) {
   +      '#uri' => \Drupal::service('file_url_generator')->generate('core/modules/media/images/icons/no-thumbnail.png'),
   

   The service should be injected.

7. +++ b/core/tests/Drupal/KernelTests/Core/File/StreamWrapperTest.php
   @@ -99,9 +99,11 @@ public function testUriFunctions() {
   +    $file_url_generator = \Drupal::service('file_url_generator');
   

   This should be converted to $this->container->get('file_url_generator');

Thanks for the reviews!

Re: #120

1. Fixed
2. Fixed
3. Fixed
4. Fixed
5. Fixed
6. Fixed
7. Fixed

Re: #121

1. Fixed
2. Fixed
3. Follow up?
4. I'll let @Berdir answer this. :-)

#120.1 I'm re-adding this change, as we moved from a string to a Url object, and needed to update this line anyway.

$variables['link'] = Link::fromTextAndUrl($link_text, $url->setOptions($options));

Thanks for the review, if that's all you found then that sounds like you're generally on board with the direction, specifically also all the cache context/output changes with relative/absolute URL's?

> See my reply to #110.3. Do we think that is realistic? If so, then all this complex code would only exist once instead of thrice!

I think we'd need to do some profiling/benchmark to see how much overhead it is to build a Url object from a string only to get the string again. With render caching maybe a bit less so on average, but this function is called quite often.

I'll also try to count how many times we call each function now, to have an idea on how common it is, although a lot of the calls are in tests in core.

Personally, I don't think having a bit more code is that relevant, it should be pretty well covered with tests, I'd rather have a good DX for developers using this API. And in this regard, I think the question is what's easier: having a single method but having to call ->toString() and sometimes absolute, although fewer and fewer times, or multiple methods, so you need to figure out which is easier to use. Again, I'll go through the usages to figure out how a single-method approach would look like. I'll also see if someone else has an opinion on this.

Now that it works, I could also try to see if it helps to reuse some parts as protected methods, that would also help if we go the route of having decorators instead of the hook (I agree that a follow-up makes sense, only downside is that we'd then no longer need the injected service later on, but it is last in the constructor and easy to remove then), as having those helper methods might help alternative implementations too. Certainly for the decorator approach a single method would be better, otherwise it is a lot of work ot override them all.