Move StreamWrappers from strings to URL objects

for the CC failure.

Also should the test coverage be expanded a little more?

Also should the test coverage be expanded a little more?

I can't find any current getExternalUrl test coverage to migrate. My personal thoughts was that since this was used by FileUrlGenerator which has explicit tests for the path using specific stream wrappers, that that was enough test coverage. The need not to edit existing tests (assuming they pass; with the exception of the test streamwrapper implementation) would prove that the change in this low-level system works as intended.

The last submitted patch, 6: drupal-3376892-6.patch, failed testing. View results →

+++ b/core/lib/Drupal/Core/StreamWrapper/PublicStream.php
@@ -47,8 +47,19 @@ public function getDirectoryPath() {
+   */
+  public function getUrl() : Url {
     $path = str_replace('\\', '/', $this->getTarget());
-    return static::baseUrl() . '/' . UrlHelper::encodePath($path);
+    // We must replace the base URL with "base:" so that link modification works correctly
+    // downstream, but we can't change static::baseUrl because it might be relied on by others.
+    $base_url = str_replace($GLOBALS['base_url'], "base:", static::baseUrl());
+    return Url::fromUri($base_url . '/' . $path);
   }

There's an encoding issue somewhere based on some of the test fails (at least for kernel tests), as well as issues with index.php in the path.

baseUrl() has support for a file_public_base_url setting, this will likely break that in some cases (\Drupal\Tests\file\Kernel\FileUrlTest::testFilesUrlWithDifferentHostName) doesn't fail, but I think that's because it doesn't contain the base url in it. If it would, it would be completely messed up with this and we should extend the test with such a case IMHO.

I think we should inline baseUrl() and possibly deprecate it here if there's no other use.

One more thing to consider here. We might want to deprecate getExternalUrl() only in D11 for D12 as we deprecate the separate interface. Modules that will want to remain compatible with D10 and D11 will need to keep the method anyway and that's going to be confusing otherwise.

Taran2L → changed the visibility of the branch 3376892-move-streamwrappers-from to hidden.

Taran2L → changed the visibility of the branch 3376892-move-streamwrappers-from to active.

..., as well as issues with index.php in the path.

This was fixed by forcing skipping the script part as it is a URL to a file and not a route.

baseUrl() has support for a file_public_base_url setting, this will likely break that in some cases (\Drupal\Tests\file\Kernel\FileUrlTest::testFilesUrlWithDifferentHostName) doesn't fail, but I think that's because it doesn't contain the base url in it. If it would, it would be completely messed up with this and we should extend the test with such a case IMHO.

Actually, it was working as before, except encoding part was missing, fixed

I think we should inline baseUrl() and possibly deprecate it here if there's no other use.

I would, as @Kingdutch suggested, add all deprecations in the follow-up

There's an encoding issue somewhere based on some of the test fails (at least for kernel tests)

Huh, this one is very interesting. Tests are checking whether generated URLs match expected value, but the value itself is "artificial", as in browser context URL like /vfs://abcde.txt does not make any sense, i.e. browser cannot act = download these anyways.

The issues is that base:/vfs://abcde.txt is being double encoded, as Url class treats /vfs://abcde.txt as path and encodes it, right

When using it without base:, there is a malformed URI, as /vfs://abcde.txt is not valid

And, finally vfs://abcde.txt does not work, as vfs:// is not in allowed protocols list.

So, I think those tests needs to be changed a bit, for example by allowing vfs:// protocol as a valid one for testing, and then it all would work properly

Made it pass all the tests, but vfs:// thing is not cool. Probably there should be a core wrapper around vfs stream wrapper, so it just works without those conditions and special treatment

Seems to need manual rebasing.