Provide internal API for special schemes and thin public wrappers for user input and non-routed URLs

I like it!

Also: I read string-parsey-magic as string-parsley-magic :D

The question is, where would this constant live: Is it Drupal\Core\Url?

I am working on this.

Still lots of places where schemes need to be replaced with constant.

Marking as needs review, since there's a patch. Although...

- * $installer_url = \Drupal\Core\Url::fromUri('base:core/install.php')->toString();
+ * $installer_url = \Drupal\Core\Url::fromUri(Url::BASE . ':core/install.php')->toString();
...
-      ->getUrlIfValidWithoutAccessCheck($uri_reference) ?: static::fromUri('base:' . $uri_reference);
+      ->getUrlIfValidWithoutAccessCheck($uri_reference) ?: static::fromUri(static::BASE . ':' . $uri_reference);

Hm. :\ Is this actually an improvement? In addition to being way more characters, the concatenation makes it a lot harder to type without errors now, and it's inconsistently using Url::BASE vs. static::BASE in places. :\

Also, this further shows inconsistency. We don't have one of these named prefixes for public:// and private:// (and definitely won't for http or https), making this now look strange...

     $data[] = [$this->getMock('...'), 'http://www.drupal.org', TRUE];
     $data[] = [$this->getMock('...'), 'https://www.drupal.org', TRUE];
     $data[] = [$this->getMock('...'), 'Invalid', FALSE];
-    $data[] = [$this->getMock('...'), 'entity:node/1', TRUE];
-    $data[] = [$this->getMock('...'), 'base:', TRUE];
-    $data[] = [$this->getMock('...'), 'base:node', TRUE];
-    $data[] = [$this->getMock('...'), 'user-path:', TRUE];
+    $data[] = [$this->getMock('...'), Url::ENTITY . ':node/1', TRUE];
+    $data[] = [$this->getMock('...'), Url::BASE . ':', TRUE];
+    $data[] = [$this->getMock('...'), Url::BASE .':node', TRUE];
+    $data[] = [$this->getMock('...'), Url::USER_PATH . ':', TRUE];
     $data[] = [$this->getMock('...'), 'public://', FALSE];
     $data[] = [$this->getMock('...'), 'public://foo.png', TRUE];
     $data[] = [$this->getMock('...'), 'private://', FALSE];

it's inconsistently using Url::BASE vs. static::BASE in places. :\

I have added constants in Url.php so its using static::BASE or static::ENTITY in Url.php class but Url::BASE in other classes.

Is this actually an improvement?

I like that it gives a place to document the schemes. Those docs aren't in the patch, but once they are, it's kind of nice to be able to use an IDE to click through the constant usage and see docs about what the semantics of that constant is.

the concatenation makes it a lot harder to type without errors now

In an IDE, constants can be autocompleted, so that reduces errors. I can see how concatenating the constant with a ':' can be annoying though and lead to errors. In any cases, there should not be many usages of these schemes. There are a lot currently, but there are issues like #2417827: Evaluate and document each use of base: in core and #2417645: Change destination query string handling to break dependence on system path open to reduce them.

Url::BASE vs. static::BASE

Per #8, that's a common idiom in PHP now and used elsewhere in Drupal including all class-defined constants. Inside the class that defines the constant or one of its subclasses, you can (and generally should) use static, while in all consuming code you refer to the class by name.

We don't have one of these named prefixes for public:// and private:// (and definitely won't for http or https), making this now look strange

Agreed we don't need it for http and https, since those are defined by non-Drupal-only internet standards. I wouldn't be opposed to creating constants for public and private, since those are Drupalisms, so could use the docs. However, those are also fairly unambiguous. But, as we've seen, base and user-path aren't self-evident, so can benefit the most from the automatic connection to docs that constants provide.

So the current patch adds Url::BASE, Url::ENTITY and Url::USER_PATH.

I wonder if Url::SCHEME_BASE, Url::SCHEME_ENTITY and Url::SCHEME_USER_PATH would be clearer?

That being said, I'm also a bit hesitant now that I see how it affects code legibility.

In the mean time, here's a patch review:

1. +++ b/core/lib/Drupal/Core/Form/ConfirmFormHelper.php
   @@ -34,7 +34,7 @@ public static function buildCancelLink(ConfirmFormInterface $form, Request $requ
   +      $url = Url::fromUri(Url::USER_PATH . $options['path'], $options);
   

   This one forgets to add the colon.

2. +++ b/core/modules/link/src/Plugin/Field/FieldWidget/LinkWidget.php
   index a12f373..420ebca 100644
   --- a/core/modules/menu_link_content/src/Entity/MenuLinkContent.php
   
   +++ b/core/modules/menu_link_content/src/Entity/MenuLinkContent.php
   index 650b3d2..3f385e4 100644
   --- a/core/modules/menu_link_content/src/MenuLinkContentInterface.php
   
   +++ b/core/modules/serialization/src/Tests/EntityResolverTest.php
   --- a/core/modules/shortcut/src/Entity/Shortcut.php
   +++ b/core/modules/shortcut/src/Entity/Shortcut.php
   
   +++ b/core/modules/system/tests/modules/form_test/src/Form/FormTestRedirectForm.php
   index ac4d9ba..61c9e84 100644
   --- a/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
   
   +++ b/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
   index e84348c..0000000
   --- a/core/modules/views/src/Plugin/views/display/DisplayPluginInterface.php
   
   +++ /dev/null
   --- a/core/modules/views/src/Plugin/views/display/DisplayRouterInterface.php
   +++ b/core/modules/views/src/Plugin/views/display/DisplayRouterInterface.php
   

   I'm pretty certain all the changes in these files are unintentional.

What surprised me a bit was that we're not using methods. The Url object actually has fromUserPathUri()/fromEntityUri(), which seemed like a great idea to me, but they are protected helper methods.

I don't quite get why we make it hard to use those, especially now that we came around and made user-path: and entity: official API's as far as I understand, unlike the way we treated base:// before.

I originally thought that those constants are mostly about internal checks when looking for a specific schema, but looking at the patch, that is doing a lot of replacements for fromUri(), which, honestly, looks worse than it did before?

And even for checks, why not add something like Url::isUserPathScheme($uri) ?

+++ b/core/includes/common.inc
@@ -501,7 +501,7 @@ function _format_date_callback(array $matches = NULL, $new_langcode = NULL) {
  * @code
- * $installer_url = \Drupal\Core\Url::fromUri('base:core/install.php')->toString();
+ * $installer_url = \Drupal\Core\Url::fromUri(Url::BASE . ':core/install.php')->toString();
  * $external_url = \Drupal\Core\Url::fromUri('http://example.com', ['query' => ['foo' => 'bar']])->toString();

This is a perfect example, instead of having to know about the constant, and that you still need to append :, why not write Url::fromBaseUri('core/install.php')?

+1 to methods. And if we have methods, then I don't know that constants in addition to the methods would have much value. In #2417567-19: Rename user-path: scheme to internal:, I'm suggesting to rename user-path to site. If we do that, then I suggest adding the following static methods to Url:

- fromBaseRelativePath()
- fromSiteRelativePath()

I don't think we need a public one for entity, because wherever we need an entity URL from code, we should be calling $entity->url() or $entity->urlInfo(). (I wouldn't mind renaming that latter one to $entity->getUrlObject(), but that's out of scope for this issue.)

The approach looks nice to me.

I think for the other method (fromSiteRelativePath() or whatever it might be called) let's just reopen #2405551: Add a method to support UIs where users enter paths instead of route names and other valid use cases. That's exactly what that issue is about, and there are already in-progress patches there to do it. Also, I think it's potentially a more complicated situation than the one in the patch here, because 'user-path' has leaked into the public API on its own (not just in the context of Url::fromUri('user-path:'...) and therefore it might not be possible to have a single method that replaces all occurrences.

1. From what I can tell, this issue is about making the protected helpers ::fromEntityUri(), ::fromUserPathUri() and ::fromRouteUri().
   Given that, strong -1 to ::fromBaseRelativePath(). It should be ::fromBaseUri(). It needs to be consistent.
2. I'm also doubtful about having one method per built-in URI scheme. Instead of \Drupal\Core\Url being a simple API with few entry points (::fromRoute(), ::fromRouteMatch() and ::fromUri()), it then becomes one with MANY entry points.
3. Also, it will say "from entity URI", "from user path URI" etc… which means you'll still have to pass in URIs with those URI schemes! Otherwise they'll be totally misnamed. So the only gain is documentation … which is probably the point?
4. It feels like we're doing this solely because our documentation system is lacking, and the only way we can document things, is by adding methods. That feels woefully wrong.

2. It has just as many entry points now, in the form of magic string prefixes. Now it has many entry points in the form of methods that you get autocompleted/suggested.
3. I don't get this, as visible in the patch, "fromUri('base:install.php') becomes "from*Something*('install.php')" so you do not need to repeat the scheme?
4. I disagree. Those methods are not for documentation, they're for developers.

Most of the uses of user-path: are in test code. The ones in standard.install are an interim until Shortcut is finished. The rest are in Shortcut itself, the link widget, the form destination handling, and Views UI. All of these can be converted to the proper API, as far as I can see.

Even if the standard.install example is removed there would still be plenty in contrib, any time code calls entity_create('menu_link_content', ...) or entity_create('shortcut', ...) or similar, right? (Basically any time you programmatically create a link, you pass in something like 'link' => array('uri' => 'user-path:node/add')).) I don't think the same API could be used to replace that because Url::fromWhatever() would return something like '/path/to/my/drupal/site/node/add' which would not work as input to the above.

#24: programmatically created links would probably not use content entities, but use either <module>.links.menu.yml files (plenty of those in HEAD) or https://www.drupal.org/project/menu_link_config.

@effulgentsia will write up and post for others' review/feedback in a bit

Part 1 is in #2417647-28: Add leading slash to paths within 'user-path:' URIs, to allow 'user-path:' URIs to point to the <none> route. Will write up part 2 as a comment in this issue tomorrow.

Committed that; re-testing.

Ahem.

Now with less purring :P

Awesome work, xjm! I think this patch is now looking great.

I basically only have a bunch of nitpicks: either nitpicky nitpicks, or wording/formatting that I think could be clearer. Leaving at NR, to get more feedback.

1. +++ b/core/lib/Drupal/Core/Url.php
   @@ -19,11 +19,83 @@
   + * - URLs for user input.
   + * - URLs for entities.
   

   IMO it's correct to use the term "URLs" here. The \Drupal\Core\Url class is about generating URLs. You feed it either routes, route matches, or URIs.

   So, while this class accepts entity URIs as input, it turns them into entity URLs as output.

   This also makes the class-level documentation friendlier, since it doesn't need to make the distinction between URIs and URLs.

   Great! :)

2. +++ b/core/lib/Drupal/Core/Url.php
   @@ -19,11 +19,83 @@
   +   * This scheme is used internally for handling routes as URIs. To get the
   

   s/handling/storing/
   or
   s/handling/serializing/

3. +++ b/core/lib/Drupal/Core/Url.php
   @@ -98,12 +170,18 @@ class Url {
   +   * @param string $route_name_or_uri
   +   *   The name of the route or URI.
   
   @@ -122,15 +200,9 @@ class Url {
   +  public function __construct($route_name_or_uri, $route_parameters = array(), $options = array()) {
   +    $this->routeName = $route_name_or_uri;
   

   This change is *extremely* confusing. The constructor is not designed to accept URIs at all. Passing a URI here would just break.

4. +++ b/core/lib/Drupal/Core/Url.php
   @@ -173,12 +249,77 @@ public static function fromRoute($route_name, $route_parameters = array(), $opti
   +   * Creates a new URL object from an unrouted, base-relative path.
   ...
   +   * Creates a new URL object from an unrouted, host-relative path.
   ...
   +   * Creates a new URL object from a route match (e.g. for the current request).
   

   s/URL object/Url object/, as we consistently do in almost all places in this class.

5. +++ b/core/lib/Drupal/Core/Url.php
   @@ -173,12 +249,77 @@ public static function fromRoute($route_name, $route_parameters = array(), $opti
   +   * site installation that do not have a Drupal route. For example, robots.txt
   +   * and core/update.php are unrouted, base-relative paths.
   

   I think we should also mention here that it can also be used to generate URLs for non-Drupal web apps that are installed inside of Drupal's root directory.

6. +++ b/core/lib/Drupal/Core/Url.php
   @@ -173,12 +249,77 @@ public static function fromRoute($route_name, $route_parameters = array(), $opti
   +   * Do not use this method for URLs that do have a Drupal route (that is, most
   +   * pages generated by Drupal) because this will circumvent the routing system.
   

   s/most/all/, right?

7. +++ b/core/lib/Drupal/Core/Url.php
   @@ -173,12 +249,77 @@ public static function fromRoute($route_name, $route_parameters = array(), $opti
   +   *   The base-relative path, for example robots.txt.
   

   I think we want single quotes around the sample parameter value, to remove all ambiguity.

8. +++ b/core/lib/Drupal/Core/Url.php
   @@ -173,12 +249,77 @@ public static function fromRoute($route_name, $route_parameters = array(), $opti
   +   *   Drupal, but within the site directory.
   

   "within the site directory" makes me think there's a typo there and you meant "within the sites directory".

   I think this should say "but within the directory in which Drupal is installed".

9. +++ b/core/lib/Drupal/Core/Url.php
   @@ -173,12 +249,77 @@ public static function fromRoute($route_name, $route_parameters = array(), $opti
   +   * This method should only be used for paths within the base directory of the
   +   * site installation that do not have a Drupal route. For example, robots.txt
   +   * and core/update.php are unrouted, base-relative paths.
   

   AFAICT this is the exact same paragraph as the one for ::fromBaseRelativePath(). That's wrong. This should be adapted because it's for generating host-relative URLs.

10. +++ b/core/lib/Drupal/Core/Url.php
    @@ -173,12 +249,77 @@ public static function fromRoute($route_name, $route_parameters = array(), $opti
    +   *   subdirectory http://example.com/drupal/, use 'index.html' for a URL to
    

    Single quotes around the sample parameter value, great!

11. +++ b/core/lib/Drupal/Core/Url.php
    @@ -173,12 +249,77 @@ public static function fromRoute($route_name, $route_parameters = array(), $opti
    +   * Use this method to create a URL from a specific route match, that is,
    +   * the route that best corresponds to a particular request. This is often
    +   * used to generate URLs based on the URL for the current request. To
    +   * generate a URL for a specific route (rather than from a request), use
    +   * Url::fromRoute().
    

    80 cols.

12. +++ b/core/lib/Drupal/Core/Url.php
    @@ -192,24 +333,28 @@ public static function fromRouteMatch(RouteMatchInterface $route_match) {
    +   *   The URI of the resource including the scheme. Several special internal
    +   *   schemes are supported. See the documentation of each for details:
    

    Several special internal schemes are supported.
    

    sounds almost unprofessional (i.e. not the writing itself, but "yo, we invented some URI schemes just for funs and giggles") to my ears — what about:

    Several Drupal-specific URI schemes are supported.
    

    or even

    Several Drupal-specific URI schemes are supported, with well-defined semantics that capture a specific intent.
    

13. +++ b/core/lib/Drupal/Core/Url.php
    @@ -192,24 +333,28 @@ public static function fromRouteMatch(RouteMatchInterface $route_match) {
    +   *   - Url::SCHEME_BASE
    +   *   - Url::SCHEME_ROUTE
    +   *   - Url::SCHEME_ENTITY
    +   *   - Url::SCHEME_USER_PATH
    

    I'd list the scheme for routes last.

Changed as per some of the suggestions in #38 (whichever I could frame better). The only change in code was for point 3.

+++ b/core/lib/Drupal/Core/Url.php
@@ -19,11 +19,83 @@
+   * @todo What exactly is the point of this scheme? See:
+   *   https://www.drupal.org/node/2418139

I am not sure what to do with this. The issue in question is already fixed: #2418139: Add a toUriString method to Url class and add a route: scheme

+++ b/core/lib/Drupal/Core/Url.php
@@ -19,11 +19,83 @@
+  /**
+   * Scheme for URIs from user input.
+   *
+   * To create a URL for paths from user input that may or may not correspond
+   * to a valid Drupal route, use Url::fromUserInput().
+   *
+   * @see static::fromUserInput()
+   * @see static::fromUserPathUri()
+   *
+   * @todo Rename this constant depending on the outcome of:
+   *   https://www.drupal.org/node/2417567
+   */
+   const SCHEME_USER_PATH = 'user-path';

@@ -227,12 +372,6 @@ public static function fromRouteMatch(RouteMatchInterface $route_match) {
-   * Note: the user-path: scheme should be avoided except when processing actual
-   * user input that may or may not correspond to a Drupal route. Normally use
-   * Url::fromRoute() for code linking to any any Drupal page.
-   *

Should we take over that you should not use user-path, if possible?

#42: HAH! That explains some weird things that I saw when debugging somewhere in the last week! +1 to fixing that weirdness.

#44: that works too, but I figured we'd want to least stress the route: URI scheme.

Reworked the documentation for base.

static:: should actually work in docs, I think, on api.drupal.org. It definitely does in classes, and should also in docs... But I don't think we advise using it according to our docs standards. Besides which, users of these methods/constants would need to use them as Url::whatever so that is I think better. So, a good change. :)

I took a quick look through the rest of the docs. Looks mostly good! A few nitpicks:

a)

+   * element and the above URLs were written within links as:
+   * - <a href="README.txt"></a> (browser resolves as '/drupal/README.txt')
+   * - <a href="/index.html"></a> (browser resolves as '/index.html')
+   *

Any HTML example code needs to be put into @code ... @endcode to render right on api.d.o I think.

b) I had a question about this section of the docs:

   * The examples above can also be generated from the wrapper methods
+   * fromBaseRelativePath() and fromHostRelativePath(), as follows:
+   * - Url::fromBaseRelativePath('README.txt') is equivalent to
+   *   Url::fromUri(Url::SCHEME_BASE . ':README.txt').
+   * - Url::fromHostRelativePath('index.html') is equivalent to
+   *   Url::fromUri(Url::SCHEME_BASE . ':/index.html').

This didn't really make it clearer to me... Can it be combined with the previous section, which told what the output of these was, so I don't have to scroll back and forth to figure it out?

c) And here's a typo just below:

+   * scheme, mo path processors are applied (including language prefixes and

mo -> no (I think?)

d)

+   * - Multilingual sites
+   * - Search engine optimization
+   * - Hiding links the user cannot access

List standards - list items end in .

e) How come some of the methods say @return this and some say something else? One is probably incorrect. If they're factory methods, @return static is probably more correct than @return this or than spelling out the class returned.
https://www.drupal.org/node/1354#types

e)

   * This method is used to create URLs for internal and external URIs with a
+   * unified API and storage format.
+   *
+   * - For URIs that have Drupal routes (that is, most pages generated by

List standards: The line before the list should end in : and there should not be a blank line in between.

f) Generally... This documentation seems very repetitive. Maybe it's not but it really seems like every method explains all the other methods. I think maybe we should just have an @see in each one to the others and leave it at that rather than repeating "On the other hand, use this other method..." in every one. I am all for good docs but this seems like overkill. Or put the explanation of which method to use on what on the main class doc block (which may have it anyway), and in the others say "See the main class docs for an explanation of which method to use for what" or something to that effect.

I also think that putting explanations on the scheme constants is less desirable than on the methods, especially if you want to encourage people to use the methods instead of the scheme constants?

g)

 +   *   The URI of the resource including the scheme. Several Drupal-specific
+   *   URI schemes are supported. See the documentation of each for details:
+   *   - Url::SCHEME_BASE
+   *   - Url::SCHEME_ENTITY
+   *   - Url::SCHEME_USER_PATH
+   *   - Url::SCHEME_ROUTE
    * @param array $options

Needs blank line before @param.

You are correct about the lists, sorry!

API issue, sure. I don't think it's a big deal though since our docs standards definitely say that after any @tag you need to put the fully namespaced class name. https://www.drupal.org/node/1354#classes

1. +++ b/core/lib/Drupal/Core/Url.php
   @@ -190,26 +304,141 @@ public static function fromRouteMatch(RouteMatchInterface $route_match) {
   +   */
   +  public static function fromUserInput($path, $options = []) {
   

   Note: we should also @throw the exception.

2. +++ b/core/lib/Drupal/Core/Url.php
   @@ -190,26 +304,141 @@ public static function fromRouteMatch(RouteMatchInterface $route_match) {
   +    if ((strpos($path, '/') !== 0) && (strpos($path, '/') !== 0) && (strpos($path, '/') !== 0)) {
   

   There is either some serious magic going on here, or someone simply forgot to use "?" and "#" Maybe also use in_array() here.
   So we need a little bit more test coverage :)

#46:

1. +++ b/core/lib/Drupal/Core/Url.php
   @@ -41,13 +41,46 @@
   +   * checking (->access() returns TRUE for URIs in this scheme). Therefore,
   

   s/in/using/ — maybe? "in" just seems strange, but perhaps that's because I'm not a native speakers.

2. +++ b/core/lib/Drupal/Core/Url.php
   @@ -41,13 +41,46 @@
   +   * multilingual sites, with search engine optimization, and with hiding links
   

   "incompatible with SEO"… I'm not sure what that means? :)

#52:

1. +++ b/core/lib/Drupal/Core/Url.php
   @@ -70,22 +71,19 @@ class Url {
   +   * scheme, mo path processors are applied (including language prefixes and
   

   "mo" :P

2. +++ b/core/lib/Drupal/Core/Url.php
   @@ -358,18 +364,65 @@ public static function fromBaseRelativePath($path, $options = array()) {
   +    return preg_replace('/^(\/)+/', '', $path);
   

   Why not ltrim($path, '/')?

3. +++ b/core/lib/Drupal/Core/Url.php
   @@ -358,18 +364,65 @@ public static function fromBaseRelativePath($path, $options = array()) {
   +   *   - Fragments, beginning with a '#'.
   

   Maybe mention jumplinks here?

#55: see #53 :)

#2417567: Rename user-path: scheme to internal: is in now.

Just the reroll. This wasn't straightforward pick HEAD vs patch, so someone should double check that I didn't botch it.

I should be able to address the remaining feedback in the next day or three, and also add some unit test coverage for Url::fromBaseRelativePath() and Url:: fromHostRelativePath().

Since #2422995: Remove the base: scheme in favour of user-path:/internal: got closed as a dup of #2417567: Rename user-path: scheme to internal:, is there really anything to do in relation to it?

Took care of @todo

--- a/core/lib/Drupal/Core/Utility/UnroutedUrlAssembler.php
+++ b/core/lib/Drupal/Core/Utility/UnroutedUrlAssembler.php
@@ -109,9 +110,7 @@ protected function buildLocalUrl($uri, array $options = []) {
     $request = $this->requestStack->getCurrentRequest();
 
     // Remove the base: scheme.
-    // @todo Consider using a class constant for this in
-    //   https://www.drupal.org/node/2417459
-    $uri = substr($uri, 5);
+    $uri = substr($uri, strlen(Url::SCHEME_BASE));

Thought about using regex or a str_replace, but though this would be the most performant (but I didn't actually profile it).

First pass at tests. Not sure if we need additional ones?

Still going to work on this to address the undone points from #59

Grrr. Pretty sure this will fail because I just noticed strlen(Url::SCHEME_BASE) != 5. Sigh; will fix in next patch.

OK, this should come up green. Everything but #51-f should be addressed. I wanted to post a clean patch before major rewrites on those docs.

Is Url::SCHEME_USER_PATH already defined? I don't see it in the patch?

Just unassigning myself because I forgot to work on it :/

Rerolled.

the patch seems to be doing a lot more than the issue summary suggests. It's not clear to me these changes are still in scope for beta/RC

#2346787: Update Url to reflect that it handles both routes and non-routed URIs is postponed on this.

Reroll.

I didn't review the patch in #76 in detail but this looks like the cause for all the errors.

We're working on 8.1 now.

Mh, I don't like the patch as it is, this is is for sure. We should simply get rid of all URL parsing stuff for actual URLs and better use one of the \Psr\Http\Message\UriInterface implementations.

Reroll of #78.

It seems as though:

a) If we're changing track, these special cases of Url should be subclasses.

b) If we're not changing track, the documentation for the static methods should be the canonical source for how the different schemes work, not the constants. So move all the substantial documentation out of the constants and into the methods, so it's in one place.

c) And in fact we don't really need the constants, because they're implementation details of the methods, and people should just use the methods as factories and not try to build their own using the constants.

This seems worth doing still.

This is an old major... This is a kinda niave reroll that seems to apply on the 8.9 and 9.x branches. Rerolling over 5 years is special but not _too_ much has changed in Url in that period other then the finalization of the 'internal:' patch watch back before 8.0. Yowza. I kinda incorporated very quickly into the patch. Most of the other conflicts where documentation conflicts which I tried to review but enough has changed in the last 5 years the doc changes could probably all use a once over.

I'm mostly using it to do a deeper review to see if it addresses the problem I ran into with inconsistent parsing between schemas. :fingerscrossed:

+++ b/core/lib/Drupal/Core/Url.php
@@ -349,8 +520,9 @@ public static function fromUri($uri, $options = []) {
+      throw new \InvalidArgumentException(SafeMarkup::format('The entity URI "@uri" is invalid. You must specify the entity id in the URL. e.g., entity:node/1 for loading the canonical path to node entity with id 1.', ['@uri' => $uri]));

SafeMarkup? Lol not much has changed indeed. Looks like this needs more testing.

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.