Active
Project:
Drupal core
Version:
main
Component:
file system
Priority:
Normal
Category:
Plan
Assigned:
Unassigned
Issue tags:
Reporter:
Created:
11 Oct 2019 at 15:08 UTC
Updated:
25 Mar 2025 at 00:24 UTC
Jump to comment: Most recent
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.
No issue created for:
[Search needed]
Comments
Comment #2
wim leersThanks 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.
Comment #9
andypostComment #11
kingdutchThis 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
Urlis 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
Comment #12
kingdutchI've created a dent of child issues. Adding a list of what else is still needed, unfortunately the end of the day has come.
Comment #13
longwave@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.
Comment #14
longwaveA 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.
Comment #15
berdirThis 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.
Comment #16
kingdutch@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.
Comment #17
kingdutchComment #18
andypostComment #19
lauriiiLooks like we need a review from framework managers on the plan.
Comment #20
larowlan@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).
Comment #21
wim leersAgreed 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?
Comment #22
larowlanYes, I did, that and stream wrappers sound like good places to start.
Removing the tag
Comment #23
smustgrave commentedShould the postponed issues be unpostponed?
Comment #24
acbramley commentedBumping this one after coming across #3376941: Replace usage of generateString with generate in NodeRSSContentTest are these ready to work on?