Splitting up #2444003: Optimize Drupal\Core\Template\Attribute. This patch deals only with the \Drupal\Template\TwigEnvironment class.

CommentFileSizeAuthor
#49 2555549-nr-bot.txt144 bytesneeds-review-queue-bot
#36 2555549-36.patch1.85 KBborisson_
#26 2555549-21.diff2 KBAki Tendo
#21 2555549-21.diff2 KBAki Tendo
#19 2555549-19.diff2.28 KBAki Tendo
#9 2555549-09.patch4.64 KBjhedstrom
#9 interdiff.txt4.54 KBjhedstrom
#6 interdiff.txt1.68 KBAki Tendo
#6 2555549-6.diff4.44 KBAki Tendo
#3 interdiff.txt1.56 KBAki Tendo
#3 2555549-3.diff4.37 KBAki Tendo
twig-environment-patch.diff4.86 KBAki Tendo

Comments

Aki Tendo created an issue. See original summary.

Status: Needs review » Needs work

The last submitted patch, twig-environment-patch.diff, failed testing.

Aki Tendo’s picture

Aki Tendo commented
StatusFileSize
new 2555549-3.diff4.37 KB
new interdiff.txt1.56 KB

Oops - got a variable reference wrong by not checking more closely after copypasting. This does prove that runtime assertions are running.

Aki Tendo’s picture

Aki Tendo commented
Status: Needs work » Needs review
geertvd’s picture

geertvd commented
Status: Needs review » Needs work

Looks good, some nitpicks:

  1. +++ b/core/lib/Drupal/Core/Template/TwigEnvironment.php
@@ -66,6 +66,8 @@ class TwigEnvironment extends \Twig_Environment {
+    assert('is_string($twig_extension_hash)', 'Arguemnt #3 must be a string');

    typo? 

  2. +++ b/core/lib/Drupal/Core/Template/TwigEnvironment.php
@@ -100,8 +102,14 @@ protected function isFresh($cache_filename, $name) {
+   * @param string $cache_filename
+   * @param string $name

    These arguments could use an extra comment line describing them

Aki Tendo’s picture

Aki Tendo commented
StatusFileSize
new 2555549-6.diff4.44 KB
new interdiff.txt1.68 KB

Typos fixed. Also, let's see if that construct assertion can be hardened to require a file path.

Aki Tendo’s picture

Aki Tendo commented
Status: Needs work » Needs review
joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Status: Needs review » Needs work

Couple of minor things but this looks great otherwise. Thanks @Aki Tendo

  1. +++ b/core/lib/Drupal/Core/Template/TwigEnvironment.php
@@ -66,6 +66,9 @@ class TwigEnvironment extends \Twig_Environment {
   public function __construct($root, CacheBackendInterface $cache, $twig_extension_hash, \Twig_LoaderInterface $loader = NULL, $options = array()) {

    should $options at the end be type hinted as well? 

  2. +++ b/core/lib/Drupal/Core/Template/TwigEnvironment.php
@@ -66,6 +66,9 @@ class TwigEnvironment extends \Twig_Environment {
+    assert('file_exists($root)', 'Argument #1 must be a legal file path.');

    Suggestion: This may be better as 'Argument #1 must be a file path which exists' Because legality doesn't really have a play here. 2¢ Also all the other assertions don't end in periods but this one does. I'm not sure the precedent here but it should be consistent either way.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt4.54 KB
new 2555549-09.patch4.64 KB

This should address #8. Regarding periods at the end, all the examples have them, so I added them here.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
commented

Bah, that page was for our test method asserts, not PHP asserts. However, most coding standard for Drupal use periods, so I think it makes sense when using assert().

Status: Needs review » Needs work

The last submitted patch, 9: 2555549-09.patch, failed testing.

Status: Needs work » Needs review

jhedstrom queued 9: 2555549-09.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 9: 2555549-09.patch, failed testing.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
commented

Fails seem unrelated:

Output: [PHP Fatal error: Uncaught exception 'PDOException' with message 'SQLSTATE[HY000]: General error: 13 database or disk is full'

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented

That was happening yesterday too.

dawehner’s picture

dawehner
Primary language German
commented

Note: This patch will probably conflict with #2568171: Upgrade to Twig 1.22 and implement our own cache class, so maybe better wait and don't waste time.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Status: Needs work » Postponed

Let's postpone on that. And reroll after its in.

Aki Tendo’s picture

Aki Tendo commented

This patch has a sister that likewise should be postponed then.

Aki Tendo’s picture

Aki Tendo commented
Status: Postponed » Needs review
StatusFileSize
new 2555549-19.diff2.28 KB

Re-roll.

Status: Needs review » Needs work

The last submitted patch, 19: 2555549-19.diff, failed testing.

Aki Tendo’s picture

Aki Tendo commented
StatusFileSize
new 2555549-21.diff2 KB

That was embarrassing. Missing ).

Aki Tendo’s picture

Aki Tendo commented
Status: Needs work » Needs review

The last submitted patch, 19: 2555549-19.diff, failed testing.

Aki Tendo’s picture

Aki Tendo commented
Issue summary: View changes

Running the patch against 8.0.0. It has been decided that runtime assertions may be included in incremental updates.

Aki Tendo queued 21: 2555549-21.diff for re-testing.

Aki Tendo’s picture

Aki Tendo commented
StatusFileSize
new 2555549-21.diff2 KB

Requing under new system

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.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

+1.

Only concern: why the "argument #N" stuff instead of specifying the name of the argument?

Aki Tendo’s picture

Aki Tendo commented

I did it that way in mimicry of how PHP handles such an error, but I'm open to suggestions on how else to format the error message. Whatever method is used needs to be consistent across the system.

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.

Aki Tendo’s picture

Aki Tendo commented
Parent issue: #2444003: Optimize Drupal\Core\Template\Attribute »

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

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.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.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

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

markhalliwell’s picture

markhalliwell
Primary language English
commented
Version: 8.5.x-dev » 8.6.x-dev
Status: Needs review » Needs work
Issue tags: +Needs reroll
borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
Status: Needs work » Needs review
Issue tags: -Needs reroll
StatusFileSize
new 2555549-36.patch1.85 KB

Fixed #28 and rerolled the patch.

dawehner’s picture

dawehner
Primary language German
commented 
+++ b/core/lib/Drupal/Core/Template/TwigEnvironment.php
@@ -56,6 +56,8 @@ class TwigEnvironment extends \Twig_Environment {
+    assert(file_exists($root), "$root must be a file path which exists.");
+    assert(is_string($twig_extension_hash), "$twig_extension_hash must be a string");

I'm curious whether we could evaluate these conditions earlier, aka. on container build time

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented

I'm not sure about #37, that sounds like a good idea, but I'm not sure how to implement this. Should we do that as a followup to add those additionally?

Aki Tendo’s picture

Aki Tendo commented

@dawehner My understanding is the configuration yaml files are immutable once the developer has finished work on their module. Hence checking their validity within the container builder as part of a configuration validation scheme become unnecessary overhead in production where new code isn't (or at least shouldn't) being written. That's why I went with an asser() check.

The purpose of the assert is to ease and speed the diagnosis of coding problems, aid our fellow developers and lower the bar for understanding the code for newcomers to the code. Consider what happens if we do move these asserts to validation in the container interface. A programmer decides to, for whatever reason, instantiate this object without using the DI system (bad practice, but it's the easiest example I can come up with so bear with me). When they hit an error from the TwigEnvironment that's going to be the code file they open first trying to locate their mistake. The assert() where it is makes it obvious the call was done incorrectly (whether or not they should be doing this is a different can of worms).

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.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.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.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.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 2555549-nr-bot.txt144 bytes

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.