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

Created on 11 October 2019, about 5 years ago
Updated 22 February 2023, almost 2 years ago

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.

🌱 Plan
Status

Active

Version

10.1

Component
File system 

Last updated 3 days ago

Created by

🇨🇭Switzerland berdir Switzerland

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • 🇳🇱Netherlands kingdutch

    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

  • 🇳🇱Netherlands kingdutch

    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.

  • 🇬🇧United Kingdom longwave UK

    @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.

  • 🇬🇧United Kingdom longwave UK

    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.

  • 🇨🇭Switzerland berdir Switzerland

    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, Split ImageStyle into the config entity and a separate event-based image processing service Needs work 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.

  • 🇳🇱Netherlands kingdutch

    @longwave Sorry about that, I tried to split it by subsystem group, though I should've 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.

  • 🇫🇮Finland lauriii Finland

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

  • 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10

    @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).

  • 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

    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?

  • 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10

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

    Removing the tag

  • 🇺🇸United States smustgrave

    Should the postponed issues be unpostponed?

Production build 0.71.5 2024