This is the follow-up to basically implement "Option 2" from #2669074: Convert file_create_url() & file_url_transform_relative() to service, deprecate it

For that to be feasible, we need to improve several underlying API's, specifically \Drupal\Core\StreamWrapper\StreamWrapperInterface::getExternalUrl() and \Drupal\image\Entity\ImageStyle::buildUrl() so that we can consistently work with Url objects and do not have to convert between strings and Url objects several times to display an image style URL.

See issue summary over there for more details, this will be expanded as we create child issues.

Remaining tasks for generateString

No issue created for:

  • ComputedFileUrl.php
  • CssCollection: CssCollectionOptimizerLazy.php, CssCollectionRenderer.php, CssCollectionRendererUnitTest.php, CssOptimizer.php, CssOptimizerUnitTest.php
  • DownloadTest.php
  • EngineTwigTest.php
  • File: File.php, FileEntityFormatterTest.php, FileUriFormatter.php, FileUrlGeneratorTest.php
  • Image: image.admin.inc, ImageTest.php, ImageUrlFormatter.php
  • JsCollection: JsCollectionOptimizerLazy.php, JsCollectionRenderer.php

Remaining files for generateAbsoluteString

[Search needed]

Comments

Berdir created an issue. See original summary.

wim leers’s picture

Thanks for creating this follow-up issue! 🙏

Future readers: there's a lot of discussion in #2669074: Convert file_create_url() & file_url_transform_relative() to service, deprecate it about this. Search that issue before doing deep dives.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

andypost’s picture

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

kingdutch’s picture

Title: [meta] Use FileUrlGenerator::generateUrl() everywhere, then deprecate generate() and generateAbsolute() » [meta] Use FileUrlGenerator::generate() everywhere, then deprecate generateString() and generateAbsoluteString()

This issue has puzzled me for a while in the past week, but it's actually a quite doable issue! We just have to fix the issue title to make it clearer. The original title was written before we decided that Url is what we care about so it's the default, and the strings are the odd-ones out.

This was decided in #2669074-166: Convert file_create_url() & file_url_transform_relative() to service, deprecate it

kingdutch’s picture

Issue summary: View changes

I've created a dent of child issues. Adding a list of what else is still needed, unfortunately the end of the day has come.

longwave’s picture

@Kingdutch please read https://www.drupal.org/docs/develop/issues/issue-procedures-and-etiquett... and let's discuss before opening any more child issues, file by file is not a preferred way of doing things.

longwave’s picture

A possible scoping is one issue for generateString and another for generateAbsoluteString, where we add the deprecations at the same time to ensure we caught all uses. From the look of the changes this won't be too hard to review with git diff --color-words.

berdir’s picture

This issue doesn't exist because there are many calls to it. It exists because some cases would currently require to convert to a string from a Url object and back about 3 times, which is bad for performance.

We need to work bottom up to support Url objects in surrounding API's. It starts with stream wrappers, another example is then \Drupal\image\Entity\ImageStyle::buildUrl(), many of those require complex BC dances as they require new methods. For ImageStyle, #2986669: Split ImageStyle into the config entity and a separate event-based image processing service would be a chance to introduce this as it already introduces a new API.

The original issue on purpose only converted calls that allowed to fix a specific issue where it was worth doing.

kingdutch’s picture

@longwave Sorry about that, I tried to split it by subsystem group, though I shouldn't have split the media tests. I was going to do one per method at first, but there were some tests which scaffolded the methods out too, so I figured it wasn't entirely a simple find/replace that could be checked with color-words, and some more granular PRs would be appreciated (as can also be seen by the difference in the patches that just pass testing and those that have some failures).

@Berdir I agree about the fact that the underlying problem is the converting back 'n forth to URL options. However, I disagree that we need to fix everything holistically and getting rid of generateString and generateAbsoluteString is not an improvement. If anything it makes it clearer where a URL object is available in a higher level method, which makes it easier to do the complex BC dances, and it discourages new uses of non-URL implementations.

My plan was to merge the Media patches into 1 and fix the test failure and then continue. But I'll merge those 3 issues and then just pause the issue for now I suppose.

kingdutch’s picture

Issue summary: View changes
andypost’s picture

lauriii’s picture

Looks like we need a review from framework managers on the plan.

larowlan’s picture

@Kingdutch asked me to respond here with framework manager hat on.

I'm not sure what's being asked of in terms of FM review - is it whether we want to rework the lower level APIs to unblock this? If so, then I think that's fine, as long as folks realize what we're signing up for - its probably too late for Drupal 11, so we would have to support any existing APIs along with any new APIS throughout the Drupal 11 cycle (at least).

wim leers’s picture

Agreed we won't be able to deprecate all these things in 10.3 to actually drop this in 11.

It's still worth doing this though, because it'll make the DX much simpler.

@larowlan It sounds like you're +1 to the principle though. Did you see @Berdir's comment in #15 that provides a concrete example?

larowlan’s picture

Yes, I did, that and stream wrappers sound like good places to start.

Removing the tag

smustgrave’s picture

Should the postponed issues be unpostponed?

acbramley’s picture

Bumping this one after coming across #3376941: Replace usage of generateString with generate in NodeRSSContentTest are these ready to work on?

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.