Problem/Motivation

Right now toString method is not callable from Twig templates because of our Sandbox policy but at least Url objects are using this.

Proposed resolution

Add toString method to the Twig sandbox policy whitelist

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#9 add_tostring_method_to-2624284-9.patch1.47 KBlauriii
#9 add_tostring_method_to-2624284-9-test-only.patch1.02 KBlauriii
#2 add_tostring_method_to-2624284-2.patch465 byteslauriii
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

lauriii created an issue. See original summary.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Druid commented
Status: Active » Needs review
FileSize
add_tostring_method_to-2624284-2.patch465 bytes
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Druid commented
Issue tags: +Quickfix
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs review » Reviewed & tested by the community 
+++ b/core/lib/Drupal/Core/Template/TwigSandboxPolicy.php
@@ -57,6 +57,7 @@ public function __construct() {
       '__toString',
+      'toString',

Given __toString() already exists, it only makes sense that toString() is treated the same.

Also: the reason some objects don't use (the magic) __toString(), is because PHP will fatal/crash dramatically if an exception is thrown from there. So while this may seem strange, it has a very good reason.

chx’s picture

chx CreditAttribution: chx commented

++ excellent idea.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented

Shall we test it in TwigSandboxTest?

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Druid commented

It seems like we don't have tests for the allows, instead we have tests for the disallowed methods. Maybe, maybe not, I don't have strong opinion.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Version: 8.0.x-dev » 8.1.x-dev
Category: Bug report » Task
Priority: Major » Normal
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

\Drupal\Tests\Core\Template\TwigSandboxTest has tests for the allows. We can just test with a Url object. Also I'm not sure this is a bug so I think we should do this for 8.1.x and not in 8.0.x which is only for bugs fixes to the stable code base.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Druid commented
Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
add_tostring_method_to-2624284-9-test-only.patch1.02 KB
add_tostring_method_to-2624284-9.patch1.47 KB

The last submitted patch, 9: add_tostring_method_to-2624284-9-test-only.patch, failed testing.

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
Status: Needs review » Reviewed & tested by the community

Beauty, thanks @lauriii

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Fixed

Committed cd4ae1b and pushed to 8.1.x. Thanks!

  • alexpott committed cd4ae1b on 8.1.x 
    Issue #2624284 by lauriii: Add toString method to Twig sandbox policy...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.