Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Beta phase evaluation
Issue category | Bug, because it leads easily to a potential broken site. |
---|---|
Issue priority | Major, because its quite an odd and hard to debug problem |
Disruption | I hope no PHP code relied on the magic conversion. Twig will continue to work |
Problem/Motivation
You cannot throw exceptions in __toString() methods, because PHP will fatal
if you do that, see http://3v4l.org/Y33D8
Currently Url::toString()
throws though an exception (route not found for example), so we potentially fatal if we have somehow created an invalid URL,
which might happen somehow.
Proposed resolution
catch the exceptions?
Remaining tasks
User interface changes
API changes
We remove the support for the magic conversions of the URL object in code, but we keep the functionality in twig.
Comment | File | Size | Author |
---|---|---|---|
#35 | interdiff.txt | 1.91 KB | dawehner |
#35 | 2416971-35.patch | 13.31 KB | dawehner |
#33 | 2416971-33.patch | 13.09 KB | dawehner |
#27 | interdiff.txt | 1.25 KB | dawehner |
#27 | 2416971-27.patch | 13.15 KB | dawehner |
Comments
Comment #1
kgoel CreditAttribution: kgoel commentedComment #2
kgoel CreditAttribution: kgoel commentedworking on this
Comment #3
pwolanin CreditAttribution: pwolanin commentedLike this?
Comment #4
dawehnerInstead of returning an empty string I would rather vote for returning something which actually helps people to debug that. Maybe a watchdog entry (which also potentially throws an exception) or simple return 'invalid-url', or similar.
Comment #5
pwolanin CreditAttribution: pwolanin commentedHow about the exception class as the return string?
Comment #6
pwolanin CreditAttribution: pwolanin commentedWell, actually - what's the use case for __toString? I accidentally cast to string? or we want to use it somplace?
Comment #7
neclimdulI think this is for Twig?
Comment #8
dawehnerAs talked in IRC with @pwolanin we could also change twig_render_var() to check for a toString() method and use that instead.
When we decide to do that I would vote to drop the magic string support again, its error prone.
Comment #9
dawehnerLet's implement the idea in twig.
For php code we should not try to randomly cast objects to strings, this is dangerous in terms of potential bugs anyway.
Comment #11
dawehnerI hope these are the places of the magic function being used.
Comment #13
dawehnerFixed in a more sane way.
Comment #15
dawehnerSome more fixes.
Comment #17
dawehnerSome more small fixes.
Comment #18
dawehnerComment #19
Wim LeersThis 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.
Could be clearer, e.g.
Tests the automatic/magic calling of a <code>toString()
method, if it exists, by Twig.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?Comment #20
Wim LeersMore accurate title since the course was apparently changed midway.
Comment #21
dawehnerAlright, fixed those points, hopefully.
Comment #22
dawehnerJust a simple rebase.
Comment #23
Wim LeersComment #25
dawehnerThis could be it.
Comment #27
dawehnerMore fixes.
Comment #28
pwolanin CreditAttribution: pwolanin commentedLooks 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.
Comment #29
dawehnerGiven that we use $url->toString() as official API everywhere in much more places, I think that method is fine.
Comment #30
dawehnerComment #31
neclimdullets RTBC again. Still looks good.
Comment #33
dawehnerJust a simple reroll.
Comment #35
dawehnerThere we go.
Comment #36
mpdonadioSo, 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()?
Comment #37
dawehnerMy thoughts!
The only thing we don't loose is that this no longer point to the wrong path. Have a look at http://d8.dev/admin/structure/views/view/content in HEAD, its pointing to
http://d8.dev/content
Comment #38
mpdonadioDid another look through this I think this is good to go. Working on a CR.
Comment #39
dawehnerThank you!
Comment #40
alexpottThis 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!
Comment #42
jbrown CreditAttribution: jbrown commentedThere are hundreds more classes with __toString(). Do we need to remove them also?
Comment #43
dawehnerWell, those not throw an exception, right?
Comment #44
jbrown CreditAttribution: jbrown commentedSounds 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 ?
Comment #45
Wim Leers#44: quoting the IS:
Comment #47
AaronBaumanPHP 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?