Remove Url::__toString()

working on this

Like this?

How about the exception class as the return string?

Well, actually - what's the use case for __toString? I accidentally cast to string? or we want to use it somplace?

I think this is for Twig?

This greatly benefits the DX & TX, by not having puzzling fatals in Twig anymore. I'd RTBC, but I have a docs question. Once that's answered/handled, RTBC.

1. +++ b/core/modules/system/src/Tests/Theme/EngineTwigTest.php
   @@ -84,6 +84,25 @@ public function testTwigLinkGenerator() {
   +   * Tests the magic url to string Twig functions.
   

   Could be clearer, e.g. Tests the automatic/magic calling of a <code>toString() method, if it exists, by Twig.

2. +++ b/core/themes/engines/twig/twig.engine
   @@ -150,6 +150,9 @@ function twig_render_var($arg) {
   +    elseif (method_exists($arg, 'toString')) {
   +      return $arg->toString();
   +    }
   
   @@ -237,6 +240,9 @@ function twig_drupal_escape_filter(\Twig_Environment $env, $string, $strategy =
   +    elseif (method_exists($string, 'toString')) {
   +      $return = $string->toString();
   +    }
   

   Perhaps document here why Twig also supports toString(), which seems weird/non-standard, but is done for good reasons. Perhaps link to that 3v4l result, or if possible, to PHP docs?

More accurate title since the course was apparently changed midway.

Looks reasonable, though it's a little sad to lose the magic.

is there a method name that we use elsewhere or that's common for this? I guess toSting() is intuitive enough.

lets RTBC again. Still looks good.

+++ b/core/modules/views_ui/src/ViewUI.php
@@ -726,7 +726,7 @@ public function renderPreview($display_id, $args = array()) {
             if (isset($path)) {
               // @todo Views should expect and store a leading /. See:
               //   https://www.drupal.org/node/2423913
-              $path = \Drupal::l($path, Url::fromUserInput('/' . $path));
+              $path = \Drupal::l($path->toString(), $path);
             }

So, the change for first arg to \Drupal::l() makes sense. The change for the second one has me scratching my head.

So, in HEAD, we had $path which was really a Url object (via $this->executable->getUrl() about 120 lines up), which got implicitly converted to a string via the magic method when it was prepended with a '/', and we then converted it back to a Url object using Url::fromUserInput(). Wuh?

The patch just uses the $path via $this->executable->getUrl() as is.

This seems the proper thing, but is there anything we are losing by not having it go through fromUserInput()?

Did another look through this I think this is good to go. Working on a CR.

This issue addresses a major bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 01382db and pushed to 8.0.x. Thanks!

There are hundreds more classes with __toString(). Do we need to remove them also?

Sounds dangerous - any function can throw an exception.

I implemented Drupal's double exception handling years ago: http://jonathanpatrick.me/blog/drupal7-exception-handling

Seems that this is another situation where the same problem can occur.

Should we implement a trait with a safe __toString implementation that wraps a try / catch ?

#44: quoting the IS:

You cannot throw exceptions in __toString() methods, because PHP will fatal
if you do that, see http://3v4l.org/Y33D8

PHP 7.4+ adds support for exception handling in magic methods.
So for D9.3.x when 7.4 is the minimum supported PHP version, we can re-add __toString() method yeah?
Is there another issue for this open already?