@Berdir mentioned this issue to me today on IRC.

Relevant code from twig.engine:

  if (is_object($arg)) {
    if (method_exists($arg, '__toString')) {
      return (string) $arg;
    }
    throw new Exception(t('Object of type "@class" cannot be printed.', array('@class' => get_class($arg))));
  }

If possible we should mention the variable name as well as the template where this is being called from - that would be a big improvement.

Files: 
CommentFileSizeAuthor
#48 exception_message_when-2031311-48.patch11.92 KBkerby70
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch exception_message_when-2031311-48.patch. Unable to apply patch. See the log in the details link for more information. View
#46 2031311-twig-objects-46.patch12.04 KBankitgarg
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
#44 2031311-twig-objects-44.patch12.1 KBravi.khetri
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
#41 interdiff.txt3.08 KBm1r1k
#41 2031311-twig-objects-41.patch13.01 KBm1r1k
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,504 pass(es). View
#37 interdiff.txt2.95 KBm1r1k
#37 2031311-38.patch9.73 KBm1r1k
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,857 pass(es). View
#35 2031311-35.patch12.68 KBm1r1k
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,790 pass(es). View
#33 interdiff.txt5.74 KBm1r1k
#33 2031311-33.patch13.31 KBm1r1k
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,272 pass(es). View
#31 2031311-31.patch9.2 KBm1r1k
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,272 pass(es). View
#31 interdiff.txt2.37 KBm1r1k
#28 2031311-28.patch8.8 KBm1r1k
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,268 pass(es), 1 fail(s), and 0 exception(s). View
#28 interdiff.txt1.01 KBm1r1k
#25 interdiff.txt1.93 KBm1r1k
#25 2031311-25.patch8.43 KBm1r1k
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,196 pass(es), 82 fail(s), and 6 exception(s). View
#23 2031311-23.patch8.45 KBm1r1k
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
#23 interdiff.txt4.97 KBm1r1k
#20 2031311-19.patch4.26 KBm1r1k
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,537 pass(es). View
#20 interdiff.txt1007 bytesm1r1k
#16 2031311-16.patch4.22 KBm1r1k
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View
#16 interdiff.txt1.78 KBm1r1k
#11 2031311-11.patch3.59 KBmariancalinro
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,725 pass(es). View
#5 Mozilla_Firefox.png53.5 KBjoelpittet

Comments

Cottser’s picture

I thought #1922304: Remove TwigReference objects in favor of a high speed implementation by using NodeVisitors more cleverly might help us here since it adds $context and $template arguments to twig_render_var(), but at least in the current patch those seem to always be empty.

Cottser’s picture

Issue summary: View changes
Issue tags: +sprint, +DX (Developer Experience)

Would love to have this one fixed before beta, tagging for http://drupaltwig.org/issues/focus.

joelpittet’s picture

This can be done a couple of ways. Either way you'll likely need a second argument passed through twig_render_var.

One way is to cast the expression node to a string and pass pass that as the second function to render_var in the node visitor:

The resulting argument will look like this:
{{ attributes }}

'Twig_Node_Expression_Name(name: \'attributes\', is_defined_test: false, ignore_strict_check: false, always_defined: false)'

or {{ 'Pages'|t }}

'Twig_Node_Expression_Filter(
  node: Twig_Node_Expression_Constant(value: \'Pages\')
  filter: Twig_Node_Expression_Constant(value: \'t\')
  arguments: Twig_Node()
)'

or {{ tab.attributes }}

'Twig_Node_Expression_GetAttr(type: \'any\', is_defined_test: false, ignore_strict_check: false, disable_c_ext: false
  node: Twig_Node_Expression_Name(name: \'tab\', is_defined_test: false, ignore_strict_check: false, always_defined: false)
  attribute: Twig_Node_Expression_Constant(value: \'attributes\')
  arguments: Twig_Node_Expression_Array()
)'

Or maybe a prettier solution is to extend Twig_Node_Expression_Function's compile to get the arguments name.
That way it will literally print out the argument as it was compiled to php in the Twig_Template

(isset($context["attributes"]) ? $context["attributes"] : null)
t("Pages")
$this->getAttribute((isset($context["tab"]) ? $context["tab"] : null), "attributes")

Sorry if this looks messy but wanted to portray the scope of this feature request. When nodes are compiled they may be wrapped in other functions before passed to twig_render_var().

Cottser’s picture

What does Twig itself do if you try to print an object without a __toString() method? Is it any more helpful? I guess we should check that first and maybe this will relate back to #1815250: Type twig variables in PHP and remove superfluous render_var() and getAttribute() calls from the generated code or maybe we need to look upstream at the exceptions in general.

joelpittet’s picture

FileSize
53.5 KB

PHP just deals with it the way PHP deals with it (in my case xdebug picked it up)

joelpittet’s picture

For your own testing fun:

mkdir twig && cd twig
curl https://gist.githubusercontent.com/joelpittet/9489996/raw/0dc15d850da11274b8ab4cf9a880e3f5564e60ee/index.php > index.php
echo '{
    "require": {
        "twig/twig": "1.*"
    }
}' > composer.json
composer update

Then visit http://d8.dev/twig

Cottser’s picture

I ran into this exception last night and the message I got actually was pretty helpful overall, but maybe it was because it was an include or something? This was in the webprofiler module (33aa7a6 on the event_dispatcher2 branch) after clicking the route name in the bottom toolbar (e.g. view.frontpage.page_1).

Twig_Error_Runtime: An exception has been thrown during the rendering of a template ("Object of type "Drupal\Core\Extension\Extension" cannot be printed.") in "@webprofiler/Profiler/table.html.twig" at line 16. in Twig_Template->displayWithErrorHandling() (line 291 of core/vendor/twig/twig/lib/Twig/Template.php).

Cottser’s picture

But I guess even still we could try to display the variable name.

dawehner’s picture

Even more important let's not hide what "printed" means. Just use something like
"@varname of type @class don't implement __toString"

mariancalinro’s picture

Assigned: Unassigned » mariancalinro
mariancalinro’s picture

Assigned: mariancalinro » Unassigned
Status: Active » Needs review
FileSize
3.59 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,725 pass(es). View

I went with extending Twig_Node_Expression_Function and sending extra arguments to the render_var function.

I just couldn't figure out how to get the argument, so I went with the node's name, if it exists. As a fallback I used the string "Object", as the only place we are using this is in the exception thrown when an object's class doesn't implement __toString(), so a generic Object of class @class doesn't implement __toString is still readable.

Berdir’s picture

A similar one, not sure if it is related or not, but at least as problematic:

Call to undefined method Drupal\Core\Field\FieldItemList::printed() in .../core/lib/Drupal/Core/Template/Attribute.php on line 115

Someone just reported that in #drupal-contribute, it happens if you for example have node.nid in your template instead of node.id (the id() mehod) or node.nid.value (=> $node->nid->value)

Berdir’s picture

Ah, this happens if you pass set a FieldItemList object as a #default_value on a form element.

steveoliver’s picture

Status: Needs review » Needs work

Not exactly sure what the next steps are, but based on Berdir's last comments it does seem this issue needs work.

m1r1k’s picture

#12 and #13 because of

  protected function createAttributeValue($name, $value) {
    if (is_array($value)) {
      $value = new AttributeArray($name, $value);
    }
    elseif (is_bool($value)) {
      $value = new AttributeBoolean($name, $value);
    }
    elseif (!is_object($value)) {
      $value = new AttributeString($name, $value);
    }
    return $value;
  }

that allows using any object, when this object has to extend AttributeValueBase or at least implement __toString() method.
Should we include it into the current patch?

m1r1k’s picture

Status: Needs work » Needs review
FileSize
1.78 KB
4.22 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View

Here is draft version of this validation.

joelpittet’s picture

@m1r1k that may conflict a tiny bit with #2285451: Create addClass() and removeClass() methods on Attribute object for merging css class names. which should get in any time now... Not entirely but just a heads up.

m1r1k queued 16: 2031311-16.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 16: 2031311-16.patch, failed testing.

m1r1k’s picture

Status: Needs work » Needs review
FileSize
1007 bytes
4.26 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,537 pass(es). View

Rerolled.
Also I'm not sure about used Exception type but I didn't find better existed Exception class.

joelpittet queued 20: 2031311-19.patch for re-testing.

m1r1k’s picture

Status: Needs review » Needs work
Issue tags: +Drupalaton 2014, +Needs tests

Will add test that it throws exception properly

m1r1k’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
4.97 KB
8.45 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View

Status: Needs review » Needs work

The last submitted patch, 23: 2031311-23.patch, failed testing.

m1r1k’s picture

FileSize
8.43 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,196 pass(es), 82 fail(s), and 6 exception(s). View
1.93 KB

Fixed tests and remove t() from exception at all because \Exception from inside of Attribute class will break render process.

m1r1k’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 25: 2031311-25.patch, failed testing.

m1r1k’s picture

Status: Needs work » Needs review
FileSize
1.01 KB
8.8 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,268 pass(es), 1 fail(s), and 0 exception(s). View

Another one fix-reroll and extra DocComment as well.

Status: Needs review » Needs work

The last submitted patch, 28: 2031311-28.patch, failed testing.

joelpittet’s picture

Nice job, only one fail and I think it's the new test, this seems to be coming together!

Another note in the Attribute class:

+++ b/core/lib/Drupal/Core/Template/Attribute.php
@@ -99,6 +102,11 @@ protected function createAttributeValue($name, $value) {
     elseif (!is_object($value)) {
       $value = new AttributeString($name, $value);
     }
+    // Provide a better exception message when invalid object is passed as value.
+    elseif (is_object($value) && !method_exists($value, '__toString')) {
+      throw new \Exception('"' . $name . '" attribute value of type "' . get_class($value) . '" can not be converted to string');
+    }

The return value expects AttributeValueBase and if if the $value object has a __toString() method then I think it would be best if we instantiate as an AttributeString object out of it's string value or it will likely fail on line 187 Attribute::__toString()'s call to a render() method of AttributeValueBase.

So maybe that can just be the else clause and move the !method_exists($value, '__toString') condition to the condition above? Or something along those lines.

m1r1k’s picture

Status: Needs work » Needs review
FileSize
2.37 KB
9.2 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,272 pass(es). View

Yeh, previous tests failed because Drupal\Core\StringTranslation\TranslationWrapper is passed as object to createAttributeValue() and not typecasted to string (even it has right method) but it has "public function render()" as AttributeValueBase has. So we had a bug in the logic but have tests only with TranslationWrapper passed as object there that doesn't throw an exception :D

dawehner’s picture

  1. +++ b/core/lib/Drupal/Core/Template/Attribute.php
    @@ -96,9 +99,17 @@ protected function createAttributeValue($name, $value) {
    +      throw new \Exception('"' . $name . '" attribute value of type "' . get_class($value) . '" can not be converted to string');
    

    Use String::format(). Do we have a better exception here? InvalidArgumentException?

  2. +++ b/core/lib/Drupal/Core/Template/TwigNodeExpressionFunction.php
    @@ -0,0 +1,51 @@
    + * A class that extends Twig_Node_Expression_Function.
    

    It would be more helpful to explain why ... or what this is doing.

  3. +++ b/core/lib/Drupal/Core/Template/TwigNodeExpressionFunction.php
    @@ -0,0 +1,51 @@
    +  protected $extra_arguments;
    ...
    +  public function __construct($name, \Twig_NodeInterface $arguments, $extra_arguments, $lineno) {
    +    parent::__construct($name, $arguments, $lineno);
    +    $this->extra_arguments = $extra_arguments;
    

    Some documentation would be cool.

  4. +++ b/core/lib/Drupal/Core/Template/TwigNodeExpressionFunction.php
    @@ -0,0 +1,51 @@
    +
    +    $this->setAttribute('name', $name);
    +    $this->setAttribute('type', 'function');
    +    $this->setAttribute('thing', $function);
    +    $this->setAttribute('needs_environment', $function->needsEnvironment());
    +    $this->setAttribute('needs_context', $function->needsContext());
    +    $this->setAttribute('arguments', array_merge($function->getArguments(), $this->extra_arguments));
    +    if ($function instanceof \Twig_FunctionCallableInterface || $function instanceof \Twig_SimpleFunction) {
    +      $this->setAttribute('callable', $function->getCallable());
    +    }
    

    Can we document here that this does not interfere with the existing variables in the $context? All that code is not necessarily trivial. Someone should also do some profiling because adding all those variables could for example increase the needed memory.

  5. +++ b/core/lib/Drupal/Core/Template/TwigNodeExpressionFunction.php
    @@ -0,0 +1,51 @@
    +  }
    +}
    

    minimal: newlinew needed

  6. +++ b/core/tests/Drupal/Tests/Core/Template/AttributeTest.php
    @@ -227,4 +228,113 @@ public function testStorage() {
    +  /**
    +   * PHPUnit's data provider callback.
    +   *
    

    Maybe something like Provides test data for testCreateAttributeValue

  7. +++ b/core/tests/Drupal/Tests/Core/Template/AttributeTest.php
    @@ -227,4 +228,113 @@ public function testStorage() {
    +  /**
    +   * PHPUnit's data provider callback to test exceptions.
    +   *
    +   * @see $this->testCreateAttributeValueException()
    +   * @return array
    +   */
    +  public function createAttributeValueExceptionProvider() {
    +    $testCases = array();
    +
    +    // Test passed object without 'class' as name.
    +    $name = $this->getRandomGenerator()->string();
    +    $value = $this->getRandomGenerator()->object();
    +    $testCases[] = array($name, $value);
    +
    +    return $testCases;
    +  }
    +
    

    It doesn't really make sense to add a provider for a single item.

  8. +++ b/core/tests/Drupal/Tests/Core/Template/AttributeTest.php
    @@ -227,4 +228,113 @@ public function testStorage() {
    +
    +  /**
    +   * Helper method to get protected createAttributeValue method.
    +   *
    +   * @return \ReflectionMethod
    +   */
    +  protected function getCreateAttributeValueMethod() {
    +    $attributeClass = new \ReflectionClass('Drupal\Core\Template\Attribute');
    +    $method = $attributeClass->getMethod('createAttributeValue');
    +    $method->setAccessible(TRUE);
    +    return $method;
    +  }
    +
     }
    

    We discussed before that it could make sense to add this into a generic functionality, though it never went to something. damiankloip even had a patch for it.

  9. +++ b/core/themes/engines/twig/twig.engine
    @@ -119,7 +119,7 @@ function twig_render_template($template_file, $variables) {
    +function twig_render_var($arg_name, $arg) {
    

    Would be great to document $arg_name

  10. +++ b/core/themes/engines/twig/twig.engine
    @@ -144,7 +144,7 @@ function twig_render_var($arg) {
    -      throw new Exception(t('Object of type "@class" cannot be printed.', array('@class' => get_class($arg))));
    +      throw new Exception(t('@arg_name of type "@class" doesn\'t implement __toString.', array('@arg_name' => $arg_name, '@class' => get_class($arg))));
    

    Let's now skip the t() method. This is absolute BS. Isn't that also a InvalidArgumentException?

m1r1k’s picture

FileSize
13.31 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,272 pass(es). View
5.74 KB

1), 10) Switched to String::format. According to SPL documentation \UnexpectedValueException makes more sense at least for me.

4) Well the only diff between these two classes is "$this->setAttribute('arguments', array_merge($function->getArguments(), $this->extra_arguments));" line and array_merge function. I don't think we need additional comments and profiling here.

8) Totally agreed and ready to help with that task, where is it? :)

joelpittet’s picture

@m1r1k thanks for all those changes!

diff --git a/core/MAINTAINERS.txt b/core/MAINTAINERS.txt
...
-- Marc Drummond 'mdrummond' http://www.drupal.org/user/360968/
...
diff --git a/core/lib/Drupal/Core/Link.php b/core/lib/Drupal/Core/Link.php

Can you post another patch after catching up with a pull from 8.0.x? Looks like some changes got in while you were making the patch.

m1r1k’s picture

FileSize
12.68 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,790 pass(es). View

Rerolled agains updated HEAD

joelpittet’s picture

Status: Needs review » Needs work

@m1r1k sorry there is still that link file that snuck in to your patch somehow.
core/lib/Drupal/Core/Link.php

Have a quick check though I think that is the only bit that snuck in extra from #31 to #35

m1r1k’s picture

Status: Needs work » Needs review
FileSize
9.73 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,857 pass(es). View
2.95 KB

Heh, my bad :)

joelpittet’s picture

@m1r1k thanks I reviewed the patch and it looks great!

Wondering if it's possible to test the exception throws produce the expected output as well?

m1r1k’s picture

Yes, I'll added annotation for this. Also will try to find test related to twig_render_var() to test new passed exception. Also think that to provide 100% test coverage we need to cover new Twig node visitor. What do you think @joelpittet

joelpittet’s picture

I'm of the opinion we only need a test for output from twig_render_var(). Though if you have something in mind for testing the NodeVisitor don't let me stop you:)

m1r1k’s picture

FileSize
13.01 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,504 pass(es). View
3.08 KB

Added expected exception text.

Was trying to add exception test for twig_render_var() but didn't find proper place. May be we just miss such tests :)
If so @joelpittet please create proper followup please.

Cottser’s picture

I'm wondering if Twig 1.16.0 will change things at all for us:

http://blog.twig.sensiolabs.org/post/91042890778/twig-1-16-0-released

removed the __toString policy check when the sandbox is disabled

jhedstrom’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

Patch in #41 no longer applies (and also appears to contain unrelated \Drupal\Core\Link\Link code).

ravi.khetri’s picture

Status: Needs work » Needs review
FileSize
12.1 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View

Re-rolled.

Status: Needs review » Needs work

The last submitted patch, 44: 2031311-twig-objects-44.patch, failed testing.

ankitgarg’s picture

Status: Needs work » Needs review
FileSize
12.04 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View

#44 Fail Fix.

Status: Needs review » Needs work

The last submitted patch, 46: 2031311-twig-objects-46.patch, failed testing.

kerby70’s picture

Issue tags: -Needs reroll
FileSize
11.92 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch exception_message_when-2031311-48.patch. Unable to apply patch. See the log in the details link for more information. View

Reroll attached, changes in twig.engine caused a patch apply failure that may need to be looked at.

kerby70’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 48: exception_message_when-2031311-48.patch, failed testing.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.