Problem

  • $GLOBALS['theme_key'] and $GLOBALS['theme'] from the parent site (test runner) are leaking into all tests.
Files: 
CommentFileSizeAuthor
#51 drupal8.test-theme-data.51.patch3.35 KBsun
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,838 pass(es).
[ View ]
#50 drupal8.test-theme-data.50.patch1.82 KBsun
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,795 pass(es), 0 fail(s), and 42 exception(s).
[ View ]
#49 drupal8.test-theme-data.49.patch1.17 KBsun
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,772 pass(es).
[ View ]
#45 drupal-test_theme-1770902-45.patch1.6 KBInternetDevels
PASSED: [[SimpleTest]]: [MySQL] 59,850 pass(es).
[ View ]

Comments

Status:Needs review» Needs work

The last submitted patch, test.theme_.0.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new2.65 KB
PASSED: [[SimpleTest]]: [MySQL] 41,050 pass(es).
[ View ]

I almost feared that.

But that said - the failing assertions in that test are actually completely unnecessary:

- The test verifies that local CSS files can be overridden by modules/themes, using the same basename of a CSS file.

- It performs a first round of system.base.css being overridden by another test dummy file. (which still passes with this patch)

- Then it performs a second round in the reverse order. This fails, because the test does not call drupal_static_reset('drupal_add_css') to clear out the existing added CSS files first, which means that the files from the first round are still registered (i.e., you register file 1, file 2, then file 2, file 1). The algorithm for applying CSS overrides in drupal_get_css(), however, simply says: "whoever added it last, wins." -- since drupal_add_css() still has the first two, and because neither their array keys nor their weights have changed, file 2 still wins.

I actually don't know why this failure is suddenly exposed with this patch, since it seems completely unrelated to the theme functionality...

Anyway, this patch blocks #1067408: Themes do not have an installation status, so it would be great to get it in ASAP.

Status:Needs review» Needs work

+++ b/core/modules/system/lib/Drupal/system/Tests/Common/CascadingStylesheetsTest.phpundefined
@@ -215,14 +215,6 @@ function testRenderOverride() {
     $this->assert(strpos($styles, $system . '/system.base.css') === FALSE, t('The overridden CSS file is not output.'));
-
-    drupal_add_css($system . '/tests/system.base.css');
-    drupal_add_css($system . '/system.base.css');
-
-    // The standard stylesheet should be the only one included.
-    $styles = drupal_get_css();
-    $this->assert(strpos($styles, $system . '/system.base.css') !== FALSE, t('The overriding CSS file is output.'));
-    $this->assert(strpos($styles, $system . '/tests/system.base.css') === FALSE, t('The overridden CSS file is not output.'));

Removing tests isn't the best solution.

Perhaps #1388546: Adding CSS or JS files twice changes weight would help?

Status:Needs work» Needs review

Well, I hoped I had provided sufficient details for why these assertions are plain simply needless. Trying once more:

They do not test any different functionality. The removed lines are exactly identical to the previous lines, just with a reversed order. The CSS processing does not care in which order the files are - as long as the basename is the same, the result is identical.

#1388546: Adding CSS or JS files twice changes weight is not relevant (- that said, the weights did not actually change in the verbose debug output, so I wonder whether that issue might be obsolete - anyway, OT)

StatusFileSize
new2.31 KB
PASSED: [[SimpleTest]]: [MySQL] 41,052 pass(es).
[ View ]

But then again, I have better things to do than to fight over completely needless test code assertions ;)

StatusFileSize
new8.02 KB
PASSED: [[SimpleTest]]: [MySQL] 41,048 pass(es).
[ View ]

Contrary to #4, combining the patch from #1388546-24: Adding CSS or JS files twice changes weight and the patch in the OP, no resets or removals, does pass.

Huh. Interesting. I'm sorry for dismissing the relevance, and thanks for proving me wrong, @tim.plunkett! :)

So I guess the proper procedure would be to get #1388546: Adding CSS or JS files twice changes weight into core first, in order to unblock this patch... That said, I'm not sure whether the entirety of those weight changes make sense; I'll comment on the other issue.

StatusFileSize
new1.72 KB
PASSED: [[SimpleTest]]: [MySQL] 41,108 pass(es).
[ View ]

Alrighty: Further debugging and testing on #1388546: Adding CSS or JS files twice changes weight revealed the actual cause for the test failure in that CSS test:

  1. It asserts that the CSS being output is identical in consecutive calls to drupal_get_css().
  2. However, drupal_get_css() calls into drupal_render(), which calls into theme(), which "lazy-initializes" the theme if it hasn't been initialized yet.
  3. Thus, the test fails, because the output is no longer identical after the first call to drupal_get_css().

Thus, I've added a slight adjustment for this in attached patch to make sure that the theme being used by tests is correctly initialized within the test already.

Passed as expected.

Would love to quickly move forward here, since this patch is the primary blocker for #1067408: Themes do not have an installation status

Status:Needs review» Reviewed & tested by the community

That's a great fix.

Version:8.x-dev» 7.x-dev
Status:Reviewed & tested by the community» Patch (to be ported)

Seems consistent with what we do elsewhere for this kind of situation.

Committed and pushed to 8.x. Thanks!

Moving to 7.x for backport.

Priority:Major» Normal

Thanks!

Decreasing priority for D7, since nothing is blocked there. Also, a backport would be sorta blocked on the backport of #1563620: All unit tests blow up with a fatal error (which brings the code-base in line).

Assigned:sun» Unassigned

Version:7.x-dev» 8.x-dev
Status:Patch (to be ported)» Needs review
StatusFileSize
new2.27 KB
FAILED: [[SimpleTest]]: [MySQL] 41,059 pass(es), 27 fail(s), and 1,202 exception(s).
[ View ]

Ahem, *cough* ;)

Turns out that my patch in #1067408: Themes do not have an installation status still throws exceptions despite this fix.

The reason for that is that the theme is re-initialized too early in TestBase::prepareEnvironment(), which means that the global theme is unset, but immediately after re-set to the very same thing - D'OH! :P

Attached patch moves the re-initialization into the ::setUp() methods of the base classes - after the database prefix has been changed (and in case of WebTestBase, after the new site has been installed), which makes a lot more sense ;)

hah, seriously did not expect that patch to pass on the very first shot, great. ;)

Ehhh, scratch that -- the comment was meant for another issue ;)

Status:Needs review» Reviewed & tested by the community

This is just a very trivial follow-up to the original patch, so I hope you don't mind if I RTBC my own patch.

Ping?

#14: drupal8.test-theme.14.patch queued for re-testing.

Status:Reviewed & tested by the community» Needs work

The last submitted patch, drupal8.test-theme.14.patch, failed testing.

#14: drupal8.test-theme.14.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+needs backport to D7, +Testing system

The last submitted patch, drupal8.test-theme.14.patch, failed testing.

Assigned:Unassigned» sun
Status:Needs work» Needs review
StatusFileSize
new2.19 KB
FAILED: [[SimpleTest]]: [MySQL] 42,163 pass(es), 2 fail(s), and 1,272 exception(s).
[ View ]

Re-rolled.

Status:Needs review» Needs work

The last submitted patch, drupal8.test-theme.23.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new3.33 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8.test-theme.25.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Geez. No idea how many more times I need to fix that bug.

The additional system_list() fix is contained in ~5 different branches/patches in the meantime.

StatusFileSize
new2.19 KB
FAILED: [[SimpleTest]]: [MySQL] 48,232 pass(es), 49 fail(s), and 0 exception(s).
[ View ]

Re-rolled.

Status:Needs review» Needs work

The last submitted patch, drupal8.test-theme.26.patch, failed testing.

Status:Needs work» Needs review

Oops, please ignore #26 - I wrongly assumed the system_list() fix was contained in #1810990: Teared down test environment leaks into parent site/test runner - but it is not.

#25 is ready.

#25: drupal8.test-theme.25.patch queued for re-testing.

#26: drupal8.test-theme.26.patch queued for re-testing.

Status:Needs review» Needs work

The last submitted patch, drupal8.test-theme.26.patch, failed testing.

Status:Needs work» Needs review

#25: drupal8.test-theme.25.patch queued for re-testing.

#26: drupal8.test-theme.26.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+needs backport to D7, +Testing system

The last submitted patch, drupal8.test-theme.26.patch, failed testing.

OK that last test run dramatically improved test results, but nothing changed in patch. Testing manually to see if I can figure this out.

Status:Needs work» Needs review
StatusFileSize
new2.18 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-test_theme-1770902-36.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

@sun, I hope you don't mind me working on this, I realize it is assigned to you, but it seems you have not looked at it in a while, and you may have good reason. I could be wasting my time, but anyway.

It is just that I have found the solution to the test failures, I believe. So here is a new patch for testing. I think the reason is a change in unit tests. With the addition of DrupalUnitTestBase, once I moved the code to initialize the theme into there from UnitTestBase, the errors went away locally.

Anyway here is the revised patch, lets see what the test bot says.

OK not all tests passing locally, but some that failed previously, do, maybe I have not moved code into the correct place, will investigate further after xmas. Have a good one people.

hhmmmm, and they pass here. That's weird, but cool.

So how do we test this...?

Also, I am not sure whether it's correct to run theme init -- maybe? Why not just unset the globals?

Sorry @chx, I can not answer your questions, really only jumped in to re-roll/fix test failures. The underlying architecture is beyond me. Why @sun did what he did and how to test I have no idea. Sorry if that is unhelpful, hopefully @sun will pipe in. Also see #14 for an explanation of the 'why?'.

Status:Needs review» Needs work
Issue tags:+needs backport to D7, +Testing system

The last submitted patch, drupal-test_theme-1770902-36.patch, failed testing.

Issue tags:+Needs reroll

Issue summary:View changes
Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new1.6 KB
PASSED: [[SimpleTest]]: [MySQL] 59,850 pass(es).
[ View ]

Reroll

Status:Needs review» Fixed
Issue tags:-needs backport to D7

I don't know which issue/patch fixed it, but the offending call to drupal_theme_initialize() no longer exists.

Thus, marking as fixed. I do not feel comfortable with backporting this change, because it changes the test environment, which in turn may cause previously passing tests to suddenly fail.

Status:Fixed» Closed (fixed)

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

Status:Closed (fixed)» Needs work

Status:Needs work» Needs review
StatusFileSize
new1.17 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,772 pass(es).
[ View ]

Re-opened for #1067408: Themes do not have an installation status

Gotcha. The theme data cannot be taken over from the test runner, because it is already post-processed, based on the currently enabled extensions.

Priority:Normal» Major
Parent issue:» #1067408: Themes do not have an installation status
StatusFileSize
new1.82 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,795 pass(es), 0 fail(s), and 42 exception(s).
[ View ]

Further work on that issue revealed that all of the other globals set by _drupal_theme_initialize() are not reset currently.

Technically we'd have to back up and restore them similar to the existing originalTheme + originalThemeKey globals.

However, perhaps we should remove those backups/restores entirely, so that the theme is simply re-initialized from scratch (if the theme is needed after restoreEnvironment() to begin with)...?

StatusFileSize
new3.35 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,838 pass(es).
[ View ]

Attached patch implements #50 — i.e., global theme variables are unset both in prepareEnvironment() and restoreEnvironment().

+++ b/core/modules/simpletest/lib/Drupal/simpletest/TestBase.php
@@ -1097,6 +1093,10 @@ private function prepareEnvironment() {
+    unset($GLOBALS['theme_info']);
+    unset($GLOBALS['base_theme_info']);
+    unset($GLOBALS['theme_engine']);
+    unset($GLOBALS['theme_path']);

+1 for this simple approach. I like it.

Small point: What do you think about just passing multiple variables in one call to unset instead? unset(one, two, three, ..)

The last submitted patch, 50: drupal8.test-theme-data.50.patch, failed testing.

@damiankloip: I would have used multiple args to unset() normally, but in this case, every single of those unset() calls needs to be removed prior to 8.0.0, and therefore, other (possibly parallel) patches can remove individual ones more easily if they're on separate lines.

Glad to see that the simpler approach came back green, so let's go with that :)

How can one RTBC this patch?

@jibran: Not sure how to interpret your question. Are you asking for tests? I don't think I want to add test coverage for this fix, because it's just a stop-gap fix... As mentioned in #54, none of these globals should exist to begin with, and we already have a range of theme system cleanup issues that are eliminating them one by one.

Status:Needs review» Reviewed & tested by the community
Issue tags:+Quick fix

@sun thanks for the explanation. We are unsetting $_GLOBALS and it is green and it'll help a critical issue so RTBC.

RE #54, ok fair enough. Happy with that. +1 for this!

Status:Reviewed & tested by the community» Fixed

Committed ad6f3ea and pushed to 8.x. Thanks!

  • Commit ad6f3ea on 8.x by alexpott: Issue #1770902 by sun, InternetDevels, mbrett5062, tim.plunkett: Theme...

Status:Fixed» Closed (fixed)

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