Exception message when trying to print an object without __toString() in a Twig template is not helpful

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().

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

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

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

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.

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)

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

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

#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?

Here is draft version of this validation.

@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.

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

Will add test that it throws exception properly

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

Another one fix-reroll and extra DocComment as well.

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.

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

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?

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? :)

@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.

Rerolled agains updated HEAD

@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

Heh, my bad :)

@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?

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

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:)

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.

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

Re-rolled.

#44 Fail Fix.

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

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

6 years ago and Twig is still a nightmare to work with in Drupal for themer. Raise that to D9 as most dev don't look on stable version.

The last patch seems to be corrupt and doesn't apply to the repository's 2015-03-19 state... it also includes some odd changes to the Drupal\Core\Link class that do not seem to be related.

Here is a rewrite.

- Adjusted for changes in Twig and phpunit, and code style.
- Reduced the need for some code duplication by overriding ::compileCallable instead of ::compile; this allows the method to call its parent.
- Instead of $node->getNode('expr')->getAttribute('name'), use (string) $node->getNode('expr'). This should (as I understand) be a string representation of the expression, and therefore works even if the expression is not a simple variable.

Some small fixups.

Misunderstood something about the function compiler. The arguments attribute apparently specifies arguments to be prepended to the arguments, so they need to come first in the signature of render_var.

Also, the string representation of the Twig node is not useful; revert to using the name attribute.

There's still several problems with this approach:

1. Adding a new argument to the *start* of a method's signature is a BC break, since it cannot be made optional. The only workaround for adding an optional argument to the start is to remove type hints, make the last argument optional, and shift the arguments if it isn't provided.

2. $node->getAttribute('name') doesn't always yield helpful things either. For example, in {{ function(arg) }}, it will provide function as the "name".

Also changing the message to something that actually explain what the problem is such as "Object XXX cannot be printed because it has no __toString method.',

Patch rebased for 9.3.x