Follow-up from #2114563: Remove TwigReference with the help of 'without' filter replacing 'show'/'hide' functions..

Documentation clean-up for the TwigEnvironment and twig.engine functions.

Files: 
CommentFileSizeAuthor
#15 2212309-twig-engine-doc-cleanup.patch9.35 KBjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,622 pass(es). View
#10 2212309-twig-engine-doc-cleanup-10.patch9.45 KBjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,433 pass(es). View
#7 interdiff.txt3.33 KBjoelpittet
#6 2212309-twig-engine-doc-cleanup-6.patch9.35 KBjoelpittet
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2212309-twig-engine-doc-cleanup-6.patch. Unable to apply patch. See the log in the details link for more information. View
#4 2212309-twig-engine-doc-cleanup-4.patch8.13 KBjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,178 pass(es). View
#3 interdiff.txt4.8 KBjoelpittet

Comments

joelpittet’s picture

joelpittet’s picture

Title: Twig Environment Doc Cleanup Follow-up » TwigExtension, TwigNodeVisitor, twig.engine Doc Cleanup Follow-up
Status: Active » Postponed
FileSize
4.8 KB

Postponing on the critical. Here's the interdiff from #2114563-84: Remove TwigReference with the help of 'without' filter replacing 'show'/'hide' functions. to apply after it get's in.

joelpittet’s picture

Title: TwigExtension, TwigNodeVisitor, twig.engine Doc Cleanup Follow-up » Drupal\Core\Template and twig.engine doc cleanup follow-up
Status: Postponed » Needs review
Issue tags: +Quick fix
FileSize
8.13 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,178 pass(es). View

Without is in, TwigReference is out and here's some doc cleanup to go with it.

Added clean-up for the whole \Drupal\Core\Template namespace because it's easy to review them to see they are all matching in the directory.

Cottser’s picture

Status: Needs review » Needs work

Looking great overall! Few nits:

  1. +++ b/core/lib/Drupal/Core/Template/TwigExtension.php
    @@ -50,15 +64,19 @@ public function getNodeVisitors() {
       }
     }
    -
    

    Nit: Great that we're removing the extra blank line from EOF here but we should also add a blank line before the end of the class declaration per https://drupal.org/node/608152#indenting.

  2. +++ b/core/lib/Drupal/Core/Template/TwigNodeVisitor.php
    @@ -26,12 +26,7 @@ function enterNode(\Twig_NodeInterface $node, \Twig_Environment $env) {
    -   * Implements Twig_NodeVisitorInterface::leaveNode().
    -   *
    -   * We use this to inject a call to render_var -> twig_render_var()
    -   * before anything is printed.
    -   *
    -   * @see twig_render
    +   * {@inheritdoc}
    

    Can we move the "We use this" comment inside the leaveNode method? I've found it really useful to know what this code is doing :)

  3. +++ b/core/lib/Drupal/Core/Template/TwigNodeVisitor.php
    @@ -47,10 +42,10 @@ function leaveNode(\Twig_NodeInterface $node, \Twig_Environment $env) {
    -   * Implements Twig_NodeVisitorInterface::getPriority().
    +   * {@inheritdoc}
        */
       function getPriority() {
    -    // We want to run before other NodeVisitors like Escape or Optimizer
    +    // We want to run before other NodeVisitors like Escape or Optimizer.
    

    This hunk can be removed since this will be taken care of in #2221535: TwigNodeVisitor's priority is too early and breaks some filters and macros.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
9.35 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2212309-twig-engine-doc-cleanup-6.patch. Unable to apply patch. See the log in the details link for more information. View

Thanks @Cottser, did each thing in #5 and a couple more including removing the unused class TwigFunctionTokenParser

joelpittet’s picture

FileSize
3.33 KB

Damn forgot my interdiff.

Cottser’s picture

Title: Drupal\Core\Template and twig.engine doc cleanup follow-up » Drupal\Core\Template and twig.engine docs, coding standards, and unused code cleanup
Status: Needs review » Reviewed & tested by the community

Damn TwigReference had its claws in everywhere! That code removal can be a separate issue if needed but other than that this all looks like good cleanup to me and I don't see much if any conflicts happening (other than the autoescape patch but should be easy to resolve those).

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 6: 2212309-twig-engine-doc-cleanup-6.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
9.45 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,433 pass(es). View

Re-rolled. that priority line was conflicting.

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community

Oh and back to rtbc.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 10: 2212309-twig-engine-doc-cleanup-10.patch, failed testing.

Cottser’s picture

Status: Needs work » Needs review
Cottser’s picture

Status: Needs review » Needs work
+++ b/core/lib/Drupal/Core/Template/TwigNodeVisitor.php
@@ -47,9 +44,10 @@ function leaveNode(\Twig_NodeInterface $node, \Twig_Environment $env) {
-   * {@inheritdoc}
+   * Implements Twig_NodeVisitorInterface::getPriority().

This needs to be reverted.

joelpittet’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
9.35 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,622 pass(es). View

Doc fix back to what it was, thanks @Cottser.

Cottser’s picture

Great, basically the same as #6 now. Thanks! +1 to RTBC.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

  • Commit a178ac6 on 8.x by webchick:
    Issue #2212309 by joelpittet: Drupal\Core\Template and twig.engine docs...

Status: Fixed » Closed (fixed)

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