Comments

lokapujya’s picture

Title: Add throws to Twig txtension comments » Add throws to Twig extension comments
joelpittet’s picture

Status: Active » Needs review
Issue tags: -D8 Accelerate +rc eligible
+++ b/core/lib/Drupal/Core/Template/TwigExtension.php
@@ -369,6 +369,9 @@ public function attachLibrary($library) {
    *   The escaped, rendered output, or NULL if there is no valid output.
+   *
+   * @throws \Exception
+   *   When the object cannot be printed.

Just curious because I don't know when this needs to be used but this one is only throwing because it uses the escapeFilter function. So does it need the same @throws or does the escapeFilter() just need it?

Status: Needs review » Needs work

The last submitted patch, AddThrowsToTwigExtensionComments.patch, failed testing.

lokapujya’s picture

Status: Needs work » Needs review
FileSize
1.3 KB

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.

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community

{@inheritthrows} :P

The last submitted patch, AddThrowsToTwigExtensionComments.patch, failed testing.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/lib/Drupal/Core/Template/TwigExtension.php
@@ -372,6 +372,9 @@ public function attachLibrary($library) {
+   * @throws \Exception
+   *   When the object cannot be printed.

@@ -400,6 +403,9 @@ public function escapePlaceholder($env, $string) {
+   *   When the object cannot be printed.

@@ -483,6 +489,9 @@ public function escapeFilter(\Twig_Environment $env, $arg, $strategy = 'html', $
+   *   When the object cannot be printed.

This exception is only thrown if the parameter is an object. Saying "the object" implies the methods are always dealing with an object - they are not.

lokapujya’s picture

Status: Needs work » Needs review
FileSize
1.36 KB
1.4 KB

Improved throws doc.

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community

It's a grab bag with using parenthesis in core or not. But this looks like it addresses @alexpott's concerns in #7 thanks @lokapujya

I'm sure that () could be added on commit.

The last submitted patch, AddThrowsToTwigExtensionComments.patch, failed testing.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

To be complete we also need to say into which param. And @joelpittet is correct we need a () on the __toString.

lokapujya’s picture

Issue tags: +Novice

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

er.manojsharma’s picture

Assigned: Unassigned » er.manojsharma
er.manojsharma’s picture

Please check updated patch

joelpittet’s picture

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

Though a bit out of scope with the () adding there, it's in the same file and this is still documentation changes only so I think this is fine.

@er.manojsharma could you also mention which param the object that is checked will throw the Exception as @alexpott asked for in #11?

Also, could you please provide an interdiff so we can easily see what changed from the previous patch. @see https://www.drupal.org/documentation/git/interdiff

lokapujya’s picture

@er.manojsharma thanks for helping out with this!

sdstyles’s picture

visabhishek’s picture

Status: Needs review » Reviewed & tested by the community

The last submitted patch, AddThrowsToTwigExtensionComments.patch, failed testing.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/lib/Drupal/Core/Template/TwigExtension.php
    @@ -372,6 +372,9 @@ public function attachLibrary($library) {
    +   *   When passed $string is an object that has no __toString() method.
    
    @@ -400,6 +403,9 @@ public function escapePlaceholder($env, $string) {
    +   *   When passed $arg is an object that has no __toString() method.
    
    @@ -483,6 +489,9 @@ public function escapeFilter(\Twig_Environment $env, $arg, $strategy = 'html', $
    +   *   When passed $arg is an object that has no __toString() method.
    

    This is not strictly true. It is thrown is $arg is an object and that object does not implement RenderableInterface or implement __toString() or implement toString().

  2. +++ 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;
    

    This line now exceeds 80 chars

priya.chat’s picture

Hello,
please review the patch as I have removed the extra space chars and also trying to change the doc comment.

lokapujya’s picture

Status: Needs review » Needs work
+++ 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.

snehi’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
23.88 KB
422 bytes

Removed all trailing spaces.
@Priya please find attached patch and see the diff where you are wrong.

snehi’s picture

Status: Reviewed & tested by the community » Needs work

lokapujya’s picture

  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.
priya.chat’s picture

Assigned: priya.chat » Unassigned
mglaman’s picture

Status: Needs work » Needs review
FileSize
1004 bytes

Here is rolled patch with feedback from #26. Only touches TwigExtension.

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community

Thanks @mglaman!

lokapujya’s picture

Status: Reviewed & tested by the community » Needs work
+++ 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.

lokapujya’s picture

Status: Needs work » Reviewed & tested by the community

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

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 28: add_throws_to_twig-2501735-28.patch, failed testing.

joelpittet’s picture

Status: Needs work » Reviewed & tested by the community

Besttot?!

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

#20.1 is still not addressed - the documentation is not strictly true. To quote the previous comment...

This is not strictly true. It is thrown is $arg is an object and that object does not implement RenderableInterface or implement __toString() or implement toString().

snehi’s picture

@Alexpott can you please give a hint what should be replaced with text Any suggestion.

lokapujya’s picture

  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.
    +   *
    
  2. $string should be $arg here.

  3. +++ 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.

  4. @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.
  5. +++ 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.

deepak_123’s picture

Assigned: Unassigned » deepak_123
malavya’s picture

Added a patch addressing @lokapujya changes from the previous patch.

malavya’s picture

Assigned: deepak_123 » malavya
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 38: add_throws_to_twig-2501735-37.patch, failed testing.

lokapujya’s picture

  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.

ChuChuNaKu’s picture

ChuChuNaKu’s picture

I've added another patch addressing @lokapujya comments in #41.

ChuChuNaKu’s picture

Assigned: ChuChuNaKu » Unassigned
lokapujya’s picture

Looks good to me. Can someone else review this?

alvar0hurtad0’s picture

Status: Needs work » Needs review

Changing the status

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community

I think this addresses @alexpott's concerns in #34. Thanks for tidying this up.

catch’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/lib/Drupal/Core/Template/TwigExtension.php
@@ -471,10 +475,10 @@ public function escapeFilter(\Twig_Environment $env, $arg, $strategy = 'html', $
+   * When $arg is passed as an object which does not implement __toString(),
+   * RenderableInterface or toString(). 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.

This comment no longer makes sense without the mention of the exception getting thrown, especially the first sentence.

Ankit Agrawal’s picture

Assigned: Unassigned » Ankit Agrawal
Ankit Agrawal’s picture

Assigned: Ankit Agrawal » Unassigned
snehi’s picture

Assigned: Unassigned » snehi
Issue tags: +drupalconasia2016
snehi’s picture

Status: Needs work » Needs review
FileSize
727 bytes
2.94 KB

Added exception things.

lokapujya’s picture

Status: Needs review » Needs work
Issue tags: -SprintWeekendBOS
+++ 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.

snehi’s picture

Sorry i did not get you.

lokapujya’s picture

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;

snehi’s picture

@lokapujya thanks for the review. PFA attached patch and interdiff.

lokapujya’s picture

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

snehi’s picture

Thanks for correcting me.

snehi’s picture

Status: Needs work » Needs review
lokapujya’s picture

Status: Needs review » Reviewed & tested by the community
catch’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Needs a re-roll for 8.1.x.

malavya’s picture

Assigned: snehi » malavya

Working on the reroll

malavya’s picture

malavya’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 63: add_throws_to_twig-2501735-63.patch, failed testing.

malavya’s picture

Version: 8.0.x-dev » 8.1.x-dev
Status: Needs work » Needs review
lokapujya’s picture

Status: Needs review » Reviewed & tested by the community

back to RTBC.

  • catch committed 7f02f89 on 8.0.x
    Issue #2501735 by snehi, lokapujya, malavya, priya.chat, sdstyles,...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.0.x, thanks!

  • catch committed 6e3370b on 8.1.x
    Issue #2501735 by snehi, lokapujya, malavya, priya.chat, sdstyles,...

Status: Fixed » Closed (fixed)

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

Status: Closed (fixed) » Needs work

The last submitted patch, 63: add_throws_to_twig-2501735-63.patch, failed testing.

andypost’s picture

andypost’s picture

Status: Fixed » Closed (fixed)

proper status