Add throws to Twig extension comments

Just escapeFilter() will satisfy PHPDoc. Since the exception isn't caught by escapePlaceholder, the developer friendly thing to do is document here too. Added throws for new function.

Improved throws doc.

To document which parameter is the object and adding the ().

@er.manojsharma thanks for helping out with this!

+++ b/core/lib/Drupal/Core/Template/TwigExtension.php
@@ -490,7 +490,7 @@ public function escapeFilter(\Twig_Environment $env, $arg, $strategy = 'html', $
    * @throws \Exception
-   *   When the object cannot be printed.
+   *   When an object is passed that has no __toString method.

I originally made the above change. But, would it be better to say something like "When an object is passed in the $string variable and it cannot be converted to a string." ? Then the more technical details about __toString() can be gotten from looking at the code.

The last patch contains more throws docs and line spacing changes (and some line spacing errors) that are not related to the issue summary.

1. +++ b/core/lib/Drupal/Core/Template/TwigExtension.php
   @@ -156,18 +156,14 @@ public function getFilters() {
          new \Twig_SimpleFilter('placeholder', [$this, 'escapePlaceholder'], array('is_safe' => array('html'), 'needs_environment' => TRUE)),
   -
          // Replace twig's escape filter with our own.
          new \Twig_SimpleFilter('drupal_escape', [$this, 'escapeFilter'], array('needs_environment' => true, 'is_safe_callback' => 'twig_escape_filter_is_safe')),
   -
          // Implements safe joining.
          // @todo Make that the default for |join? Upstream issue:
          //   https://github.com/fabpot/Twig/issues/1420
          new \Twig_SimpleFilter('safe_join', [$this, 'safeJoin'], ['needs_environment' => true, 'is_safe' => ['html']]),
   -
          // Array filters.
          new \Twig_SimpleFilter('without', 'twig_without'),
   -
   

   Out of scope.

2. +++ b/core/lib/Drupal/Core/Template/TwigExtension.php
   @@ -550,9 +553,9 @@ public function safeJoin(\Twig_Environment $env, $value, $glue = '') {
        return implode($glue, array_map(function($item) use ($env) {
   -      // If $item is not marked safe then it will be escaped.
   -      return $this->escapeFilter($env, $item, 'html', NULL, TRUE);
   -    }, (array) $value));
   +          // If $item is not marked safe then it will be escaped.
   +          return $this->escapeFilter($env, $item, 'html', NULL, TRUE);
   +        }, (array) $value));
   

   out of scope.

3. @@ -30,6 +30,9 @@ class ContextualController implements ContainerAwareInterface {
       *
       * @return \Symfony\Component\HttpFoundation\JsonResponse
       *   The JSON response.
   +   *
   +   * @throws \Symfony\Component\HttpKernel\Exception\BadRequestHttpException
   +   *   Thrown when the contextual IDs are not found.
   

   Out of scope, since this ticket was just for TwigExtension. If you want those other throws docs, I think that a new issue should be opened because now all those have to be reviewed in order to get this issue to RTBC.

4. Also, we need to decide what the text should be. See #20 and #22.

+++ b/core/lib/Drupal/Core/Template/TwigExtension.php
@@ -483,6 +486,9 @@ public function escapeFilter(\Twig_Environment $env, $arg, $strategy = 'html', $
+   *   When passed $string as an object that has no __toString() method.

This sentence doesn't make sense anymore.

And, I thought we were keeping the toString() updates. See comment #15.

NM, it sounds ok (I misread it) and the toString() updates can be done separate.

1. @@ -483,6 +486,9 @@ public function escapeFilter(\Twig_Environment $env, $arg, $strategy = 'html', $
   +   *   When passed $string as an object that has no __toString() method.
   +   *
   

$string should be $arg here.

2. +++ b/core/lib/Drupal/Core/Template/TwigExtension.php
   @@ -431,7 +437,7 @@ public function escapeFilter(\Twig_Environment $env, $arg, $strategy = 'html', $
   -      // You can't throw exceptions in the magic PHP __toString methods, see
   +      // You can't throw exceptions in the magic PHP __toString() methods, see
   
   @@ -509,7 +518,7 @@ public function renderVar($arg) {
   -      // You can't throw exceptions in the magic PHP __toString methods, see
   +      // You can't throw exceptions in the magic PHP __toString() methods, see
   

   I think we should make these documentation changes also since they are related and in the same file. But if you make these changes watch out for the 80 character limit.

3. @alexpott: How about one of these:
   A. When $arg is an object and that object does not implement RenderableInterface or implement __toString() or implement toString().
   B. When an object is passed in the $arg variable that is not a render array and cannot be converted to a string.

4. +++ b/core/lib/Drupal/Core/Template/TwigExtension.php
   @@ -469,7 +475,7 @@ public function escapeFilter(\Twig_Environment $env, $arg, $strategy = 'html', $
   -   * If an object is passed that has no __toString method an exception is thrown;
   +   * If an object is passed that has no __toString() method an exception is thrown;
   

   Although doing #2 opens up the fact that this one will probably need the same wording change that we make to the @throws doc.

1. +++ b/core/lib/Drupal/Core/Template/TwigExtension.php
   @@ -403,7 +403,8 @@ public function escapePlaceholder($env, $string) {
   -   *   When passed $string as an object that has no __toString() method.
   +   *   When $arg is passed as an object which does not implement __toString(),
   +   *   RenderableInterface or toString().
   
   @@ -474,9 +475,9 @@ public function escapeFilter(\Twig_Environment $env, $arg, $strategy = 'html', $
   +   * An exception is throw if an object is passed without a __toString() method;
   

   this should be the same as the throws doc.

2. +++ b/core/lib/Drupal/Core/Template/TwigExtension.php
   @@ -517,9 +519,9 @@ public function renderVar($arg) {
   -      // You can't throw exceptions in the magic PHP __toString methods, see
   -      // http://php.net/manual/en/language.oop5.magic.php#object.tostring so
   -      // we also support a toString method.
   +      // Since you cannot throw exceptions from within PHP method __toString(),
   +      // see http://php.net/manual/en/language.oop5.magic.php#object.tostring
   +      // so we also support a toString method.
   

   Please revert the re-wording of this sentence; it is no longer a complete sentence. Just add the ().

The patch did not apply.

Looks good to me. Can someone else review this?

+++ b/core/lib/Drupal/Core/Template/TwigExtension.php
@@ -477,6 +477,7 @@ public function escapeFilter(\Twig_Environment $env, $arg, $strategy = 'html', $
+   * If it already implements __toString(), it will throw an exception.

I am not sure this is needed?

The review from #48 was because the sentence used to be like "If an object is passed ... an exception is thrown.", but got changed in #43. That part was fine, it's just the "which does not implement __toString(), RenderableInterface or toString()" part that should be the same as the throws doc.

I think the @throws comment changes are good, but the regular method comment:

+++ b/core/lib/Drupal/Core/Template/TwigExtension.php
@@ -471,10 +475,11 @@ public function escapeFilter(\Twig_Environment $env, $arg, $strategy = 'html', $
-   * If an object is passed that has no __toString, method an exception is thrown;
-   * other objects are casted to string. However in the case that the object is an
-   * instance of a Twig_Markup object it is returned directly to support auto
-   * escaping.
+   * When $arg is passed as an object which does not implement __toString(),
+   * RenderableInterface or toString(). Other objects are casted to string.
+   * If it already implements __toString(), it will throw an exception.
+   * However in the case that the object is an instance of a Twig_Markup object
+   * it is returned directly to support auto escaping.

Instead of this, shouldn't we just change that first sentence like below (and adjust it to fix the 80 chars.) ?
* If an object is passed which does not implement __toString(), RenderableInterface or toString() method an exception is thrown;

1. +++ b/core/lib/Drupal/Core/Template/TwigExtension.php
   @@ -471,10 +475,12 @@ public function escapeFilter(\Twig_Environment $env, $arg, $strategy = 'html', $
   +   * Other objects are casted to string. If it already implements
   +   * __toString(), it will throw an exception. However in the case that the
   

   I don't get why we are adding "If it already implements __toString(), it will throw an exception."

2. +++ b/core/lib/Drupal/Core/Template/TwigExtension.php
   @@ -471,10 +475,12 @@ public function escapeFilter(\Twig_Environment $env, $arg, $strategy = 'html', $
   +   * If an object is passed which does not implement __toString(),
   +   * RenderableInterface or toString() method an exception is thrown;
   

   Sorry, I know this is what I gave you, but something still sound funny here. I think the word method should be replaced with "then")? If we keep "method", it should be "the __toString() method" and "the toString() method".

back to RTBC.