Problem

Goal

  • Improve Drupal core's test coverage and reduce/eliminate differences between a site installed via Simpletest vs. manual installation.

Details

  • Originally, this was done for performance reasons. Which means that we implemented performance optimizations into the custom Simpletest installation routine, instead of improving the performance of the non-interactive installer.
  • The Drupal installer got heavily improved meanwhile. It supports a non-interactive flag now, which shortcuts most installation steps.
  • Instead of a different, custom installation procedure, WebTestBase::setUp() should just call the non-interactive installer.

Proposed solution

  1. Replace WebTestBase::setUp()'s installation routine with the regular, non-interactive Drupal installer.

Dependencies / spin-offs

Changes in this patch, which should be backported:

Related issues

Files: 
CommentFileSizeAuthor
#119 1215104_we_love_php.patch529 byteschx
PASSED: [[SimpleTest]]: [MySQL] 41,486 pass(es).
[ View ]
#115 test.setup_.115.patch22.27 KBsun
PASSED: [[SimpleTest]]: [MySQL] 41,470 pass(es).
[ View ]
#115 interdiff.txt642 bytessun
#109 test.setup_.109.patch22.27 KBsun
PASSED: [[SimpleTest]]: [MySQL] 41,294 pass(es).
[ View ]
#109 interdiff.txt1.05 KBsun
#107 test.setup_.107.patch22.29 KBsun
FAILED: [[SimpleTest]]: [MySQL] 41,291 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#107 interdiff.txt4.33 KBsun
#101 test.setup_.101.patch22.81 KBsun
PASSED: [[SimpleTest]]: [MySQL] 41,227 pass(es).
[ View ]
#99 test.setup_.99.patch22.67 KBsun
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch test.setup_.99.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#99 interdiff.txt1.3 KBsun
#97 test.setup_.97.patch22.84 KBeffulgentsia
PASSED: [[SimpleTest]]: [MySQL] 41,205 pass(es).
[ View ]
#95 test.setup_.94.patch22.61 KBsun
PASSED: [[SimpleTest]]: [MySQL] 41,091 pass(es).
[ View ]
#93 test.setup_.93.patch22.6 KBsun
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch test.setup_.93.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#93 interdiff.txt6.39 KBsun
#91 test.setup_.91.patch28.99 KBsun
PASSED: [[SimpleTest]]: [MySQL] 41,082 pass(es).
[ View ]
#85 test.setup_.85.patch29.07 KBsun
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch test.setup_.85.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#85 interdiff.txt1.16 KBsun
#84 test.setup_.84.patch29.05 KBsun
PASSED: [[SimpleTest]]: [MySQL] 40,546 pass(es).
[ View ]
#84 interdiff.txt1.61 KBsun
#82 test.setup_.82.patch28 KBsun
FAILED: [[SimpleTest]]: [MySQL] 40,423 pass(es), 4 fail(s), and 4 exception(s).
[ View ]
#82 interdiff.txt3.87 KBsun
#80 test.setup_.80.patch24.79 KBsun
FAILED: [[SimpleTest]]: [MySQL] 40,511 pass(es), 29 fail(s), and 5 exception(s).
[ View ]
#80 interdiff.txt9.24 KBsun
#78 test.setup_.78a.patch18.68 KBsun
FAILED: [[SimpleTest]]: [MySQL] 40,415 pass(es), 5 fail(s), and 1 exception(s).
[ View ]
#78 test.setup_.78b.patch16.73 KBsun
FAILED: [[SimpleTest]]: [MySQL] 40,386 pass(es), 29 fail(s), and 43 exception(s).
[ View ]
#78 interdiff.a-b.txt2.67 KBsun
#78 interdiff.78b.txt2.43 KBsun
#71 test.setup_.71.patch16.44 KBsun
FAILED: [[SimpleTest]]: [MySQL] 39,701 pass(es), 26 fail(s), and 0 exception(s).
[ View ]
#71 interdiff.txt1.65 KBsun
#69 test.setup_.69.patch14.96 KBsun
FAILED: [[SimpleTest]]: [MySQL] 39,742 pass(es), 25 fail(s), and 0 exception(s).
[ View ]
#63 test.setup_.63.patch15.11 KBsun
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch test.setup_.63.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#58 test.setup_.57.without-block.patch15.12 KBsun
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch test.setup_.57.without-block.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#57 test.setup_.57.patch16.25 KBsun
PASSED: [[SimpleTest]]: [MySQL] 39,762 pass(es).
[ View ]
#56 test.setup_.56.patch21.36 KBsun
PASSED: [[SimpleTest]]: [MySQL] 37,084 pass(es).
[ View ]
#55 1215104-simpletest-install_drupal-55-D7-do-not-test.patch20.9 KBlucascaro
#53 test.setup_.52.patch21.35 KBsun
PASSED: [[SimpleTest]]: [MySQL] 37,278 pass(es).
[ View ]
#53 interdiff.txt685 bytessun
#52 1215104-simpletest-install_drupal-52.patch20.78 KBlucascaro
PASSED: [[SimpleTest]]: [MySQL] 37,279 pass(es).
[ View ]
#51 1215104-simpletest-install_drupal-51.patch278.11 KBlucascaro
PASSED: [[SimpleTest]]: [MySQL] 37,282 pass(es).
[ View ]
#51 interdiff.txt842 byteslucascaro
#49 test.setup_.49.patch20.68 KBsun
FAILED: [[SimpleTest]]: [MySQL] 37,279 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#49 interdiff.txt2.37 KBsun
#47 drupal8.test-setup.47.patch21.31 KBsun
FAILED: [[SimpleTest]]: [MySQL] 37,201 pass(es), 38 fail(s), and 10 exception(s).
[ View ]
#47 interdiff.txt2.22 KBsun
#46 drupal8.test-setup.46.patch21.32 KBsun
FAILED: [[SimpleTest]]: [MySQL] 37,202 pass(es), 38 fail(s), and 10 exception(s).
[ View ]
#46 interdiff.txt1.72 KBsun
#43 drupal8.test-setup.43.patch20.18 KBsun
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
#43 interdiff.txt11.37 KBsun
#41 drupal8.test-setup.41.patch22.57 KBsun
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
#41 interdiff.txt7.35 KBsun
#40 interdiff.txt4.26 KBsun
#36 drupal8.test-setup.36.patch20.69 KBlucascaro
FAILED: [[SimpleTest]]: [MySQL] 37,207 pass(es), 37 fail(s), and 12 exception(s).
[ View ]
#33 drupal8.test-setup.33.patch20.51 KBlucascaro
FAILED: [[SimpleTest]]: [MySQL] 37,161 pass(es), 41 fail(s), and 1,227 exception(s).
[ View ]
#20 drupal8.test-setup.20.patch18.58 KBsun
FAILED: [[SimpleTest]]: [MySQL] 36,897 pass(es), 83 fail(s), and 11,772 exception(s).
[ View ]
#18 drupal8.test-setup.18.patch13.35 KBsun
FAILED: [[SimpleTest]]: [MySQL] 37,163 pass(es), 81 fail(s), and 64 exception(s).
[ View ]
#16 drupal8.test-setup.16.patch10.44 KBsun
FAILED: [[SimpleTest]]: [MySQL] 37,157 pass(es), 81 fail(s), and 64 exception(s).
[ View ]
#14 drupal8.test-setup.14.patch11.68 KBsun
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#11 drupal8.test-setup.9.patch.txt6.94 KBsun
#5 drupal8.test-setup.5.patch3.92 KBsun
FAILED: [[SimpleTest]]: [MySQL] 631 pass(es), 414 fail(s), and 826 exception(es).
[ View ]

Comments

beejeebus’s picture

i like this concept. even if this adds overhead, tests are stupidly slow right now anyway, so we might as well at least have consistency if we can't have speed.

David_Rothstein’s picture

Subscribing.

I am trying to decide if this is a duplicate of #630446: Allow SimpleTest to test the non-interactive installer or a prerequisite. Especially since this is filed as a bug report (which I kind of agree with), I'm leaning towards prerequisite :)

dealancer’s picture

The sollution is to allow some tests to install Drupal with non-interactive flag, while other tests should go as it is now.

This will make almost all tests to rum faster, while other will user normail site installer.

sun’s picture

Status:Active» Needs review
StatusFileSize
new3.92 KB
FAILED: [[SimpleTest]]: [MySQL] 631 pass(es), 414 fail(s), and 826 exception(es).
[ View ]

Initial attempt.

This patch throws a kinda weird fatal error when trying to run a test:

ResponseText: PDOException: SQLSTATE[42S02]: Base table or view not found: 1146 Table 'test_drupal8.simpletest741293node_access' doesn't exist: SELECT COUNT(*) FROM {node_access};
in node_requirements() (line 4042 of \core\modules\node\node.module).

Essentially, "Class 'Database' not found in _registry_check_code()" - i.e., in the very function that's supposed to auto-load classes ;)

EDIT: Caused by: #1679594: node_requirements() breaks Drupal installer

Status:Needs review» Needs work

The last submitted patch, drupal8.test-setup.5.patch, failed testing.

David_Rothstein’s picture

To deal with the performance issue, maybe it's possible to keep some version of $this->preloadRegistry() around? I guess that would require convincing the Drupal installer not to balk if the registry tables already exist when it tries to install, but it seems like that might be doable.

Also cross-linking this issue with my comment at #1052692-51: New import API for major version upgrades (was migrate module)... If we can get install_drupal() working inside the context of an already-running site, it would probably help that issue also.

sun’s picture

In light of and compared to all of the Testing system improvements I'm currently working on, that preloadRegistry() hack looks like a negligible and poor premature performance optimization to me.

My goal is to streamline and actually "de-hack" the entire testing environment. Only once that is done, we can look into additional performance optimization hacks (on top of the huge list of improvements) - if, and in case that's still necessary then after all.

beejeebus’s picture

as the author of the preloadRegistry() hack, i say kill it. kill it dead.

beejeebus’s picture

oh i forgot - when we put that in, the registry was still scanning the known universe, and building entries for functions etc.

that's no longer true, so it's no longer the gain it once was.

sun’s picture

StatusFileSize
new6.94 KB

The new classloader resolves issues like in #5. :)

Re-rolled against HEAD. This blows up my local web server, so intentionally no .patch suffix.

sun’s picture

If I get the error right: The installer fails, because the batch of the Drupal installer is mixed into the SimpleTest batch of the parent site (executing the test).

Batch in a batch, anyone?

lucascaro’s picture

What about doing a fake batch call if we detect other batch jobs when installing?
something like should do it but it's a hacky hack:

<?php
\
     
$batch =& batch_get();
       
$progressive = $batch['progressive'];
       
$batch['progressive'] = FALSE;
        if (
count($batch['sets']) > 1) {
       
$current_set = $batch['current_set'];
         
$batch['current_set'] = 1;
         
//Fake batch processing since we are already in a batch job!
         
_batch_populate_queue($batch, 1);
          require_once
DRUPAL_ROOT . '/includes/batch.inc';
         
_batch_process();
         
// Reset batch status.
         
$batch['current_set'] =$current_set;
        }
       
$batch['progressive'] = $progressive;
?>

(in function install_run_task($task, &$install_state) )
What do you think? too ugly?

sun’s picture

Status:Needs work» Needs review
StatusFileSize
new11.68 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

This one should work.

Status:Needs review» Needs work

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

sun’s picture

Assigned:Unassigned» sun
Status:Needs work» Needs review
StatusFileSize
new10.44 KB
FAILED: [[SimpleTest]]: [MySQL] 37,157 pass(es), 81 fail(s), and 64 exception(s).
[ View ]

oh my. PIFR doesn't like changes to the Drupal installer... :(

Status:Needs review» Needs work

The last submitted patch, drupal8.test-setup.16.patch, failed testing.

sun’s picture

Status:Needs work» Needs review
StatusFileSize
new13.35 KB
FAILED: [[SimpleTest]]: [MySQL] 37,163 pass(es), 81 fail(s), and 64 exception(s).
[ View ]

Attached patch makes both run-tests.sh and the Simpletest UI work -- by backing up Simpletest's batch before invoking install_drupal() and restoring it afterwards.

The only remaining issue is that the Simpletest batch does not redirect to the test results page, even though it finishes successfully. It looks like something in the installer destroys that info (or alternatively Simpletest's batch ID/record).

Status:Needs review» Needs work

The last submitted patch, drupal8.test-setup.18.patch, failed testing.

sun’s picture

Status:Needs work» Needs review
StatusFileSize
new18.58 KB
FAILED: [[SimpleTest]]: [MySQL] 36,897 pass(es), 83 fail(s), and 11,772 exception(s).
[ View ]

Even when completely skipping Batch API for the non-interactive installer, the batch-finished page of the Simpletest batch still redirects to the front page.

The reason for this is that Batch API validates that the requested batch ID belongs to the current user via drupal_get_token() and that relies on
- the session ID
- the site's private key
- the global $drupal_hash_salt / $databases

The token generated after test execution is different to the stored/expected one.

This means that install_begin_request() or something else in the installer and/or WebTestbase changes too many environment variables that affect the token generation.

I tried to backup + reset pretty much everything already, with no luck.
(all attempts are still in this patch)

sun’s picture

Extracted the TestBase database connection fixes into #1679570: TestBase does not always use the correct database connection for handling assertions

Extracted the node_requirements() fix into #1679594: node_requirements() breaks Drupal installer

Status:Needs review» Needs work

The last submitted patch, drupal8.test-setup.20.patch, failed testing.

lucascaro’s picture

Hey @sun I was debugging the installation process for a while and I think I know where the problem is:

The postSave function for UserStorageController does:

<?php
     
// If the password has been changed, delete all open sessions for the
      // user and recreate the current one.
     
if ($entity->pass != $entity->original->pass) {
       
drupal_session_destroy_uid($entity->uid);
        if (
$entity->uid == $GLOBALS['user']->uid) {
         
drupal_session_regenerate();
        }
      }
?>

And that last drupal_session_regenerate is what is breaking the session id and therefore, changing the token. The call stack for this is:

core/modules/user/lib/Drupal/user/UserStorageController.php.Drupal\user\UserStorageController->postSave:169
core/modules/entity/lib/Drupal/entity/DatabaseStorageController.php.Drupal\entity\DatabaseStorageController->save:427
core/modules/user/lib/Drupal/user/UserStorageController.php.Drupal\user\UserStorageController->save:83
core/modules/entity/lib/Drupal/entity/Entity.php.Drupal\entity\Entity->save:231
core/includes/install.core.inc.install_configure_form_submit:1961
core/includes/form.inc.form_execute_handlers:1470
core/includes/form.inc.drupal_process_form:868
core/includes/form.inc.drupal_form_submit:706
core/includes/install.core.inc.install_run_task:454
core/includes/install.core.inc.install_run_tasks:382
core/includes/install.core.inc.install_drupal:85
core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.php.Drupal\simpletest\WebTestBase->setUp:675
core/modules/field/lib/Drupal/field/Tests/FieldTestBase.php.Drupal\field\Tests\FieldTestBase->setUp:30
core/modules/field/lib/Drupal/field/Tests/BulkDeleteTest.php.Drupal\field\Tests\BulkDeleteTest->setUp:88
core/modules/simpletest/lib/Drupal/simpletest/TestBase.php.Drupal\simpletest\TestBase->run:525
core/modules/simpletest/simpletest.module._simpletest_batch_operation:181
core/includes/batch.inc.call_user_func_array:289
core/includes/batch.inc._batch_process:289
core/includes/batch.inc._batch_do:166
core/includes/batch.inc._batch_page:85
core/modules/system/system.admin.inc.system_batch_page:2305
core/lib/Drupal/Core/EventSubscriber/LegacyControllerSubscriber.php.call_user_func_array:45
core/lib/Drupal/Core/EventSubscriber/LegacyControllerSubscriber.php.Drupal\Core\EventSubscriber\{closure}:45
core/vendor/symfony/http-kernel/Symfony/Component/HttpKernel/HttpKernel.php.call_user_func_array:128
core/vendor/symfony/http-kernel/Symfony/Component/HttpKernel/HttpKernel.php.Symfony\Component\HttpKernel\HttpKernel->handleRaw:128
core/vendor/symfony/http-kernel/Symfony/Component/HttpKernel/HttpKernel.php.Symfony\Component\HttpKernel\HttpKernel->handle:72
index.php.{main}:40

So basically, using drupal_install, calls install_configure_form_submit() which in turn creates a new account and does:

<?php
$account
->save();
?>

and destroys the session.

Technically, we could overcome this problem if we temporarily changed $GLOBALS['user']->uid or, even better, if we could set $entity->original->pass to $entity->pass.

We need to figure out if we really need to regenerate the session when creating the user on the installation process, and then we could patch install_configure_form_submit() or maybe doing some juggling with $GLOBALS['user']->uid in WebTestBase::setUp()...

sun’s picture

I'm sorry, but this needs to be stated:

@lucascaro++

Seriously, THANK YOU for stepping through the debugger for this! :)

That's incredibly helpful. That said, I'm not yet sure why my attempts to backup + restore the current user's session(_id) + stuff didn't have an effect (though possibly, all of that needs to be backed up + restored like $this->originalUser is in the master TestBase::setUp() and ::tearDown(), instead of right after install_drupal()...)

lucascaro’s picture

@sun no problem!

I think that drupal_session_regenerate() invalidates the old session id in the database:

<?php
 
if (isset($old_session_id)) {
   
$params = session_get_cookie_params();
   
$expire = $params['lifetime'] ? REQUEST_TIME + $params['lifetime'] : 0;
   
setcookie(session_name(), session_id(), $expire, $params['path'], $params['domain'], $params['secure'], $params['httponly']);
   
$fields = array('sid' => session_id());
    if (
$is_https) {
     
$fields['ssid'] = session_id();
     
// If the "secure pages" setting is enabled, use the newly-created
      // insecure session identifier as the regenerated sid.
     
if (variable_get('https', FALSE)) {
       
$fields['sid'] = $session_id;
      }
    }
   
db_update('sessions')
      ->
fields($fields)
      ->
condition($is_https ? 'ssid' : 'sid', $old_session_id)
      ->
execute();
  }
?>

so maybe the only option is to avoid that session regeneration?

I will try a few options and report back.

lucascaro’s picture

Hey, wait... this is using the prefixed database so the session should be still working for the batch... *gets confused by simpletest* right?

lucascaro’s picture

Ok, quick update, session_id with parameters can only be used before session_start so it would not work for restoring the session id. so there are 2 options:

* Updating the session id for the old session in the database, and the token for the batch process or
* Avoid regenerating the session by setting the original password in the installer.

I'd go with the second option if this does not represent a problem for other reasons, since technically we're not changing the user's password (and I think we can get away with not regenerating the session when installing a new site, right?)

lucascaro’s picture

Well, adding

<?php
  $account
->original = new stdClass();
 
$account->original->pass = $account->pass;
?>

before

<?php
  $account
->save();
?>

worked but there's another problem, with user_login_finalize() which does drupal_session_regenerate(); If I disable that session regenerate, the batch process works but I'm afraid it could break the installation process...

So back to restoring stuff on WebTestCase.php, probably is the best option, but we need to update session ids and tokens for batch processes.

lucascaro’s picture

There's also the problem of the cache that generates the failure in the tests above (http://qa.drupal.org/pifr/test/299058).

This is failing because on install, the cache class is set to InstallBackend which is a fake cache with dummy functions and does not save anything in the cache.

This class gets saved in the static $cache_objects in cache() (cache.inc: 26) and since this is not using drupal_static "because we do not want to change the storage of a cache bin mid-request" it keeps using the fake cache even after resetting the cache_classes variable.

@sun: do you think it is possible to perform the installation unsing $this->drupalPost('install.php') or something like that? maybe running though all the installation steps and clicking on the submit button?
This way we would be doing new requests and we would solve the session/batch problem as well as the cache issue...

moshe weitzman’s picture

Non-interactive install is a feature of Drupal that merits testing just like interactive install. It is heavily used by drush users, jenkins jobs, etc. The title of this issue suggests that we favor exercising one install method over the other.

lucascaro’s picture

@moshe weitzman indeed, although in this case we're not testing the installation process itself but trying to install drupal to run tests, but I see your point.

I think the issue here is that simpletest is using a completely different method to install drupal, reproducing a variation of the install process that in some cases does not work like neither the interactive install or the non interactive install, so that's what needs to be fixed.

lucascaro’s picture

Oh, also, using drupalGet wouldn't work because it needs the database.

lucascaro’s picture

StatusFileSize
new20.51 KB
FAILED: [[SimpleTest]]: [MySQL] 37,161 pass(es), 41 fail(s), and 1,227 exception(s).
[ View ]

It seems to me we need (at least) this change:

+++ b/includes/cache.inc
@@ -16,13 +16,12 @@
function cache($bin = 'cache') {
+  $cache_objects = &drupal_static(__FUNCTION__, array());
+
   // Temporary backwards compatibiltiy layer, allow old style prefixed cache
   // bin names to be passed as arguments.
   $bin = str_replace('cache_', '', $bin);

-  // We do not use drupal_static() here because we do not want to change the
-  // storage of a cache bin mid-request.
-  static $cache_objects;
   if (!isset($cache_objects[$bin])) {
     $class = variable_get('cache_class_' . $bin);
     if (!isset($class)) {
@@ -30,6 +29,7 @@ function cache($bin = 'cache') {

@@ -30,6 +29,7 @@ function cache($bin = 'cache') {
     }
     $cache_objects[$bin] = new $class($bin);
   }

from #1297136: Use DrupalNullCache for the installer (originally at #1082328: Provide a proper no-op cache.inc) to make the cache work and then something like the attached patch would work (let's see what the testbot says).

I've also had to add

// Build a dummy 'context' array to avoid exceptions.
        $batch_context = array(
          'sandbox'  => &$current_set['sandbox'],
          'results'  => &$current_set['results'],
          'finished' => &$finished,
          'message'  => &$task_message,
        );

Before calling _install_module_batch to avoid exceptions and solved the batch problem regenerating and updating the tokens in the database, doing on startUp() (at the very beginning):

<?php
   $batch
= batch_get();
   
$this->batch_id = $batch['id'];
   
$this->old_token = drupal_get_token($this->batch_id);
?>

and then at the end of tearDown():

<?php
//Regenerate batch token after install_drupal created a new session_id.
   
db_update('batch')
      ->
fields(array(
       
'token' => drupal_get_token($this->batch_id),
      ))
      ->
condition('token', $this->old_token)
      ->
execute();
?>

This obviously needs a lot of cleanup but we need to come up with something that works first ;)

So far this is passing my small subset of tests and I'll let testBot decide whether this passes or not all of the tests for core :)

lucascaro’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, drupal8.test-setup.33.patch, failed testing.

lucascaro’s picture

Status:Needs work» Needs review
StatusFileSize
new20.69 KB
FAILED: [[SimpleTest]]: [MySQL] 37,207 pass(es), 37 fail(s), and 12 exception(s).
[ View ]

now with support for non-batched tests! (tested with drush test-run)

Status:Needs review» Needs work

The last submitted patch, drupal8.test-setup.36.patch, failed testing.

sun’s picture

@moshe weitzman: It's not clear to me what you tried to say. This issue is about replacing Simpletest's custom code for installing the Drupal site (under test) with the regular non-interactive Drupal installer. The Drupal installer is not tested at all currently.

Please note that testing of the interactive installer is left for #630446: Allow SimpleTest to test the non-interactive installer. It cannot be tested currently, because the installer contains an explicit check for the HTTP user agent and denies further execution on a positive match. This happens for security reasons, because anyone would be able to install unlimited numbers of testing sites on all production sites on the net. That's a very complex topic on its own, so the discussion on the interactive installer should ideally stay on that issue.

Thus, the interactive installer cannot be used for this issue (and also should not be used, since the internal browser is considerably slower than the direct script execution).

@lucascaro: Thanks for pushing this forward! I'll be away the next days, but will try to review your changes as soon as possible.

moshe weitzman’s picture

Title:Simpletest setUp installs Drupal differently; use non-interactive install instead» Use non-interactive install in DrupalWebTestCase::setUp()

Ah, OK. I misunderstood the issue. Removing the first part since it is too vague (different from what?).

sun’s picture

StatusFileSize
new4.26 KB

Well, just different. :) It's an entirely custom installation procedure that shortcuts and redoes each and everything that the installer normally does with regard to requirements, install profile, module installation, and initial setup. It is randomly calling into some functions, and hopes/assumes that the result is going to be "similar" to the one of the regular installer. That's different. ;)

Attached is an interdiff between #20 and #36.

Given that we found the actual cause and reason for why the batch (token) gets corrupted, I guess that all the changes to install_run_task() for shortcutting the non-interactive/non-progressive batch case can be reverted.

Most of the remaining test failures are still cache related. The cause for that is relatively clear: install_begin_request() sets/overrides global $conf options to enable the NullBackend. This means we need to backup $conf before the installation and restore it afterwards.

I'll quickly try to do those adjustments.

sun’s picture

Status:Needs work» Needs review
StatusFileSize
new7.35 KB
new22.57 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

So... apparently, we already have a mechanism for this, which is drupal_save_session(FALSE).

The problem is that drupal_save_session() is not checked for the session regenerate and destroy cases.

I'm totally not sure whether this is valid and legit, but when injecting the identical condition that happens in the session commit and other callbacks into the regenerate and destroy callbacks, then this patch magically starts to work! :)

Perhaps @moshe weitzman can tell us whether this is utterly wrong or even a bugfix of its own...? :)

#23 explains that the Drupal installer is saving the $user for uid 1 to the (new) database, and is automatically logged in as the last step of the installation process. While everything happens in the child site under test, the PHP session handling happens in the global scope. Therefore, this $user->save() as well as the user_login_finalize() regenerate/destroy the session ID + session + cookies of the user that is running the test through the Simpletest UI. That, in turn, changes the value of drupal_get_token(), which Batch API saves and validates for each batch to be executed.

I'll attach the full patch including all debugging comments and stuff so far + interdiff to this comment, and clean up and reduce the whole thing to the required changes in my next comment (so we can get back to this version, in case these session.inc changes are invalid).

Status:Needs review» Needs work

The last submitted patch, drupal8.test-setup.41.patch, failed testing.

sun’s picture

Status:Needs work» Needs review
StatusFileSize
new11.37 KB
new20.18 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

And here's the cleaned up patch with proper code and comments and stuff. :)

Status:Needs review» Needs work

The last submitted patch, drupal8.test-setup.43.patch, failed testing.

sun’s picture

Status:Needs work» Needs review

For the record: This patch actually passed. The remaining test failures and fatal errors are actual errors in HEAD.

sun’s picture

StatusFileSize
new1.72 KB
new21.32 KB
FAILED: [[SimpleTest]]: [MySQL] 37,202 pass(es), 38 fail(s), and 10 exception(s).
[ View ]

Should fix all fatals.

sun’s picture

StatusFileSize
new2.22 KB
new21.31 KB
FAILED: [[SimpleTest]]: [MySQL] 37,201 pass(es), 38 fail(s), and 10 exception(s).
[ View ]

Moved the drupal_save_session(FALSE) call into TestBase::prepareEnvironment(), which is where the global $user is backed up. It is re-enabled in TestBase::tearDown(), so it makes sense to keep all of that in TestBase.

Status:Needs review» Needs work

The last submitted patch, drupal8.test-setup.47.patch, failed testing.

sun’s picture

Status:Needs work» Needs review
StatusFileSize
new2.37 KB
new20.68 KB
FAILED: [[SimpleTest]]: [MySQL] 37,279 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Attached patch should hopefully pass.

547b963 Fixed form_process_checkboxes()/form_type_checkboxes_value() hack.
a4e1f9f Fixed full revert of global $conf breaks rebuild operations that depend on the (default) theme.

Status:Needs review» Needs work

The last submitted patch, test.setup_.49.patch, failed testing.

lucascaro’s picture

Status:Needs work» Needs review
StatusFileSize
new842 bytes
new278.11 KB
PASSED: [[SimpleTest]]: [MySQL] 37,282 pass(es).
[ View ]

Hey @sun that's almost there, that one missing test seems to be related to drupal_save_session and, while debugging, I found that it's reset to TRUE in drupal_cron_run. See stack trace:

core/includes/session.inc.drupal_save_session:528
core/includes/common.inc.drupal_cron_run:5279
core/includes/install.core.inc.install_finished:1661
core/includes/install.core.inc.install_run_task:530
core/includes/install.core.inc.install_run_tasks:390
core/includes/install.core.inc.install_drupal:85
core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.php.Drupal\simpletest\WebTestBase->setUp:667
core/modules/system/lib/Drupal/system/Tests/Session/SessionTest.php.Drupal\system\Tests\Session\SessionTest->setUp:22
core/modules/simpletest/lib/Drupal/simpletest/TestBase.php.Drupal\simpletest\TestBase->run:525
core/modules/simpletest/simpletest.module._simpletest_batch_operation:181
core/includes/batch.inc.call_user_func_array:286
core/includes/batch.inc._batch_process:286
core/includes/batch.inc._batch_do:163
core/includes/batch.inc._batch_page:82
core/modules/system/system.admin.inc.system_batch_page:2305
core/lib/Drupal/Core/EventSubscriber/LegacyControllerSubscriber.php.call_user_func_array:45
core/lib/Drupal/Core/EventSubscriber/LegacyControllerSubscriber.php.Drupal\Core\EventSubscriber\{closure}:45
core/vendor/symfony/http-kernel/Symfony/Component/HttpKernel/HttpKernel.php.call_user_func_array:128
core/vendor/symfony/http-kernel/Symfony/Component/HttpKernel/HttpKernel.php.Symfony\Component\HttpKernel\HttpKernel->handleRaw:128
core/vendor/symfony/http-kernel/Symfony/Component/HttpKernel/HttpKernel.php.Symfony\Component\HttpKernel\HttpKernel->handle:72
index.php.{main}:40

core/includes/common.inc.drupal_cron_run:5279 calls

<?php
  drupal_save_session
(TRUE);
?>

and is executed after the drupal_save_session(FALSE) in TestBase. The thing here is that prepareEnvironment is being executed before install_drupal

When I re add drupal_save_session(FALSE) below install_drupal, this doesn't fail anymore. Here's a patch with this change.

lucascaro’s picture

StatusFileSize
new20.78 KB
PASSED: [[SimpleTest]]: [MySQL] 37,279 pass(es).
[ View ]

The last patch had some garbage from my tests, sorry about that. Here's the same patch without the test files.

sun’s picture

StatusFileSize
new685 bytes
new21.35 KB
PASSED: [[SimpleTest]]: [MySQL] 37,278 pass(es).
[ View ]

oh, wow - drupal_cron_run() is Doing It Wrong™ ! ;)

drupal_save_session() contains a pointer to the handbook page http://drupal.org/node/218104 that explains how to use it properly (which, alas, should really be documented on the API function instead).

026de92 Fixed drupal_cron_run() resets drupal_save_session().

sun’s picture

Issue summary:View changes

Rewrote the issue summary.

sun’s picture

Issue summary:View changes

Updated issue summary.

sun’s picture

lucascaro’s picture

Just an update, here's a work in progress attempt to port this patch to D7.

sun’s picture

StatusFileSize
new21.36 KB
PASSED: [[SimpleTest]]: [MySQL] 37,084 pass(es).
[ View ]

Re-rolled against HEAD.

Still waiting for the dependencies to get committed. (see issue summary)

sun’s picture

StatusFileSize
new16.25 KB
PASSED: [[SimpleTest]]: [MySQL] 39,762 pass(es).
[ View ]

All of the dependencies landed! :)

sun’s picture

StatusFileSize
new15.12 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch test.setup_.57.without-block.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

That said, I wonder whether the block schema change is actually required for this patch to pass, or whether that was caused by some other hiccup only.

In case this patch passes, we most likely should consider to do that schema change nevertheless -- but in a separate issue.

moshe weitzman’s picture

Status:Needs review» Reviewed & tested by the community

Nice code cleanup. No UI impact.

sun’s picture

Title:Use non-interactive install in DrupalWebTestCase::setUp()» Use the non-interactive installer in WebTestBase::setUp()

This patch essentially tests the Drupal installer (implicitly) with every web test case.

Due to that, I'd ideally like this patch to go in before #1599108: Allow modules to register services and subscriber services (events) -- as it's entirely not clear to me at this point, whether the kernel patches are taking the Drupal installer properly into account.

For full disclosure though I need to amend that this patch slows down the total time the testbot needs for the Drupal core suite by ~3 minutes. However, that only means we should work on the installer's performance — there are a lot of optimization possibilities in that code, and doing so will not only benefit the test suite, but also every single Drupal site that is being installed.

aspilicious’s picture

Issue tags:-Testing system

#58: test.setup_.57.without-block.patch queued for re-testing.

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

The last submitted patch, test.setup_.57.without-block.patch, failed testing.

sun’s picture

Status:Needs work» Needs review
StatusFileSize
new15.11 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch test.setup_.63.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Merged HEAD.

Status:Needs review» Needs work

The last submitted patch, test.setup_.63.patch, failed testing.

sun’s picture

Status:Needs work» Needs review

meh. Exactly what I guessed:

exception 'Symfony\Component\DependencyInjection\Exception\RuntimeException' with message 'You tried to create a service of an inactive scope.'

:-(

effulgentsia’s picture

sun’s picture

Issue tags:-Testing system

#63: test.setup_.63.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+Testing system

The last submitted patch, test.setup_.63.patch, failed testing.

sun’s picture

Status:Needs work» Needs review
StatusFileSize
new14.96 KB
FAILED: [[SimpleTest]]: [MySQL] 39,742 pass(es), 25 fail(s), and 0 exception(s).
[ View ]

Merged in HEAD.

Status:Needs review» Needs work

The last submitted patch, test.setup_.69.patch, failed testing.

sun’s picture

Status:Needs work» Needs review
StatusFileSize
new1.65 KB
new16.44 KB
FAILED: [[SimpleTest]]: [MySQL] 39,701 pass(es), 26 fail(s), and 0 exception(s).
[ View ]

Resolved the new test failures thanks to the help of @effulgentsia!

Fixed language() tries to access language.manager service outside of request scope.
Fixed t() attempts to invoke locale() even if Locale module is not installed.

Status:Needs review» Needs work

The last submitted patch, test.setup_.71.patch, failed testing.

sun’s picture

Status:Needs work» Needs review

Alright, the remaining test failures seem to be two-fold:

1) update.php requires to override drupal_container() in the same way as install.php/install_begin_request() is doing (but there's not even a comparable function/entry-point, so it's going to be hard to figure out the proper place for A) applying the override and B) reverting it as soon as that is possible).

2) Many unit tests seem to be failing. Because test assertion methods are calling into t(). So my suggested fix for t() in #71 might not be correct, as it adds a check for module_exists('locale') into t() in order to check whether A) the locale() function exists and B) whether its database tables exist. The call to module_exists() triggers calls to system_list(), which tries to query the database for enabled modules, but unit tests do not have a database.

Crell’s picture

It was recently decided to remove t() from assertion messages anyway: #500866: [META] remove t() from assert message

So that problem itself isn't a blocker here, but committing that patch arguably is.

moshe weitzman’s picture

The hard coded string array in settings.php works regardless of locale module. So it is untrue that t() does nothing if locale is disabled.

bforchhammer’s picture

Status:Needs review» Needs work

Coming from #1738330: Confusing Language negotiation when accessing / it would be nice if it was possible to change setup parameters like the langcode... looks like that's not possible in patch #71 at the moment. So maybe we can move the $settings variable to a class field, or add a respective parameter to setUp()?

bforchhammer’s picture

Status:Needs work» Needs review

Sorry, didn't mean to change the status.

sun’s picture

StatusFileSize
new2.43 KB
new2.67 KB
new16.73 KB
FAILED: [[SimpleTest]]: [MySQL] 40,386 pass(es), 29 fail(s), and 43 exception(s).
[ View ]
new18.68 KB
FAILED: [[SimpleTest]]: [MySQL] 40,415 pass(es), 5 fail(s), and 1 exception(s).
[ View ]

78a) Fixed module_list() and module_load_all() are reset with every drupal_static_reset(), breaking unit tests.

78b) Reverted module_load_all() $has_run flag to drupal_static(), since only module_list() cannot use drupal_static().

Status:Needs review» Needs work

The last submitted patch, test.setup_.78b.patch, failed testing.

sun’s picture

Status:Needs work» Needs review
Issue tags:+upgrade path
StatusFileSize
new9.24 KB
new24.79 KB
FAILED: [[SimpleTest]]: [MySQL] 40,511 pass(es), 29 fail(s), and 5 exception(s).
[ View ]

Turns out the entire upgrade path to D8 is b0rked!

When Locale module was enabled in D7, then update.php ends in an error, because update_prepare_d8_bootstrap() is invoked with all modules enabled and loaded, and t() is being called deeper in there, and because Locale module exists but the database tables haven't been fully upgraded yet, the updater ends in an error. The module list is reduced to system.module only slightly later in update.php when the actual updates are being executed.

All of this got revealed due to the replacement of drupal_static() with a plain static in module_list() in this patch, which is required, because otherwise any call to drupal_static_reset() in a unit test would reset that module_list() static and thus, the next call into it would attempt to retrieve the list of enabled modules via system_list() from the database -- but there is no database for unit tests.

After fixing that - and also removing the suppression of errors in update.php - further testing of LanguageUpgradePath revealed that various module update functions are calling into module API functions, which should not be available at all in the first place. Thus, also fixed those module updates.

Revert "Reverted module_load_all() $has_run flag to drupal_static(), since only module_list() cannot use...
Fixed module list in update.php and upgrade path to D8.

This one should come back green.

Status:Needs review» Needs work

The last submitted patch, test.setup_.80.patch, failed testing.

sun’s picture

Status:Needs work» Needs review
StatusFileSize
new3.87 KB
new28 KB
FAILED: [[SimpleTest]]: [MySQL] 40,423 pass(es), 4 fail(s), and 4 exception(s).
[ View ]

...adding this to my Top 10 of Most Horrible Commit Messages Ever™:

Fixed update.php requires full module list for certain operations, but not for the actual update environment.

Status:Needs review» Needs work

The last submitted patch, test.setup_.82.patch, failed testing.

sun’s picture

Status:Needs work» Needs review
StatusFileSize
new1.61 KB
new29.05 KB
PASSED: [[SimpleTest]]: [MySQL] 40,546 pass(es).
[ View ]

Fixed t() tries to invoke locale() during update.php requirements checks.

(also added docs to t() for this clusterfuck)

sun’s picture

StatusFileSize
new1.16 KB
new29.07 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch test.setup_.85.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

GREEEN! :)

Final tweak:

Performance: Make t() check for the primary locale() condition first.

sun’s picture

I'm checking whether update.php upgrade path fixes could be extracted into an own issue, or whether they depend on the module_list() + drupal_static() changes that are required for this patch.

sun’s picture

Extracted the update.php upgrade path fixes into this long-standing epic:
#922996: Upgrade path is broken since update.php does not enforce empty module list

aspilicious’s picture

This patch has 500 test passes LESS than the referenced one in the last comment. Thats weird....
Is it possible some tests don't get run, or is there something else going on?

sun’s picture

#85: test.setup_.85.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+upgrade path, +Testing system

The last submitted patch, test.setup_.85.patch, failed testing.

sun’s picture

Status:Needs work» Needs review
StatusFileSize
new28.99 KB
PASSED: [[SimpleTest]]: [MySQL] 41,082 pass(es).
[ View ]

Merged in HEAD.

sun’s picture

There's some pushback on the update.php changes that have been extracted into #922996: Upgrade path is broken since update.php does not enforce empty module list

So let's not hold up this patch on that. This means that we need to revert those changes and "unfix" the LanguageUpgradePathTest, which likely means that we have to implant a stop-gap fix somewhere to temporarily make it pass the tests. I'm not sure where exactly that would have to happen, but we need to figure it out.

sun’s picture

Issue tags:-upgrade path
StatusFileSize
new6.39 KB
new22.6 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch test.setup_.93.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Reverted update.php and upgrade path changes.

Status:Needs review» Needs work

The last submitted patch, test.setup_.93.patch, failed testing.

sun’s picture

Status:Needs work» Needs review
StatusFileSize
new22.61 KB
PASSED: [[SimpleTest]]: [MySQL] 41,091 pass(es).
[ View ]

Merged in HEAD.

sun’s picture

Turns out that the adjustments to module_list() + module_load_all() + t() that happened later here effectively make this patch pass the tests already.

Thus, no further changes are required to work around the LanguageUpgradePathTest failures for this issue. RTBC? :)

effulgentsia’s picture

StatusFileSize
new22.84 KB
PASSED: [[SimpleTest]]: [MySQL] 41,205 pass(es).
[ View ]

Just a reroll for HEAD changes in TestBase::tearDown().

effulgentsia’s picture

Just some questions: none of it necessarily warrants a "needs work".

+++ b/core/includes/batch.inc
@@ -439,18 +439,17 @@ function _batch_finished() {
+    // Clean-up the session. Not needed for CLI updates.
+    if (isset($_SESSION)) {
+      unset($_SESSION['batches'][$batch['id']]);
+      if (empty($_SESSION['batches'])) {
+        unset($_SESSION['batches']);
+      }
+    }
   }
   $_batch = $batch;
   $batch = NULL;

-  // Clean-up the session. Not needed for CLI updates.
-  if (isset($_SESSION)) {
-    unset($_SESSION['batches'][$batch['id']]);
-    if (empty($_SESSION['batches'])) {
-      unset($_SESSION['batches']);
-    }
-  }
-

I don't understand why this got moved into the if ($batch['progressive']) { block. There might be an obvious reason, but I don't know the Batch API well enough to say whether this makes sense.

+++ b/core/includes/bootstrap.inc
@@ -2584,7 +2605,12 @@ function drupal_fast_404() {
+  // tests from using the non-interactive installer, in which case Drupal is
+  // only happens to be installed within the same request, but subsequently

"Drupal is only happens to be" doesn't sound quite right.

+++ b/core/includes/install.core.inc
@@ -94,19 +94,27 @@ function install_drupal($settings = array()) {
+  // After execution, all tasks might be complete, in which case
+  // $install_state['installation_finished'] is TRUE. In case the last task
+  // has been processed, remove the global $install_state, so other code can
+  // reliably check whether it is running during the installer.
+  // @see drupal_installation_attempted()
+  $state = $install_state;
+  unset($install_state);

Comment says to do this in case it's the last task, but it's being done with no if statement. What if it's not the last task? Does removing $install_state cause problems then?

+++ b/core/includes/install.core.inc
@@ -240,11 +250,10 @@ function install_begin_request(&$install_state) {
   // The user agent header is used to pass a database prefix in the request when
   // running tests. However, for security reasons, it is imperative that no
   // installation be permitted using such a prefix.
-  if (isset($_SERVER['HTTP_USER_AGENT']) && strpos($_SERVER['HTTP_USER_AGENT'], "simpletest") !== FALSE) {
+  elseif (isset($_SERVER['HTTP_USER_AGENT']) && strpos($_SERVER['HTTP_USER_AGENT'], "simpletest") !== FALSE) {

Comment says this is important for security, but we're now limiting it to interactive mode only. Are we sure we don't need this security in non-interactive mode?

+++ b/core/modules/system/lib/Drupal/system/Tests/Upgrade/UpgradePathTestBase.php
@@ -222,36 +222,30 @@ abstract class UpgradePathTestBase extends WebTestBase {
-    drupal_static_reset();

Why is this being removed?

sun’s picture

StatusFileSize
new1.3 KB
new22.67 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch test.setup_.99.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Thanks for the review! :)

why this got moved into the if ($batch['progressive']) { block. There might be an obvious reason

If you check the old code flow closely, then you see that it tries to access $batch['id'] after the line that set $batch = NULL;

I stepped through the Batch API code and the batch ID only seems to be registered in the session for progressive batches.

Comment says this is important for security, but we're now limiting it to interactive mode only. Are we sure we don't need this security in non-interactive mode?

Yes, the non-interactive installer can only be executed if you have access to execute PHP on the server. Also, all that code is doing is to check for a certain HTTP header, which obviously doesn't exist in the first place when executing the non-interactive installer.

Why is this [drupal_static_reset()] being removed?

drupal_flush_all_caches() is calling drupal_static_reset() already and is invoked right after this line, so the additional static reset is needless.

Adjusted for the other issues:

Fixed grammar in drupal_installation_attempted().
Added installation_finished condition before removal of global $install_state.

Status:Needs review» Needs work

The last submitted patch, test.setup_.99.patch, failed testing.

sun’s picture

Status:Needs work» Needs review
StatusFileSize
new22.81 KB
PASSED: [[SimpleTest]]: [MySQL] 41,227 pass(es).
[ View ]

geez, HEAD is moving fast... (and it was one of my own patches that caused conflicts :P)

effulgentsia’s picture

Status:Needs review» Reviewed & tested by the community

Thanks.

sun’s picture

Issue tags:-Testing system

#101: test.setup_.101.patch queued for re-testing.

sun’s picture

Issue tags:+Testing system

#101: test.setup_.101.patch queued for re-testing.

catch’s picture

Status:Reviewed & tested by the community» Needs work
+++ b/core/includes/bootstrap.incundefined
@@ -1448,6 +1448,27 @@ function bootstrap_hooks() {
+
+  // Check whether locale() is available and operational.
+  if (!isset($locale_exits)) {
+    // The availability of locale() requires more explanation:
+    // - Whether the module system has been loaded.
+    //   In early bootstrap, in the installer, update.php, and also in early
+    //   error/exception conditions, the module system might not be loaded yet.
+    // - Whether Locale module is enabled.
+    //   The mere existence of locale() does not imply that Locale module is
+    //   actually enabled and its database tables are installed. Since PHP code
+    //   cannot be unloaded, this is typically the case in the environment that
+    //   is executing a test.
+    // - Whether Locale module actually has been loaded and locale() exists.
+    //   The Locale module may be enabled, but the actual .module might not be
+    //   loaded. This is typically the case in update.php, which needs to
+    //   populate the full module list to invoke hook_requirements(), but must
+    //   not load any actual modules and APIs, since they are not guaranteed to
+    //   be operational.
+    $locale_exists = function_exists('locale') && function_exists('module_exists') && module_exists('locale');
+  }

@@ -1469,7 +1490,7 @@ function t($string, array $args = array(), array $options = array()) {
     $string = $custom_strings[$options['langcode']][$options['context']][$string];
   }
   // Translate with locale module if enabled.
-  elseif ($options['langcode'] != LANGUAGE_SYSTEM && ($options['langcode'] != 'en' || variable_get('locale_translate_english', FALSE)) && function_exists('locale')) {
+  elseif ($options['langcode'] != LANGUAGE_SYSTEM && ($options['langcode'] != 'en' || variable_get('locale_translate_english', FALSE)) && $locale_exists) {
     $string = locale($string, $options['context'], $options['langcode']);
   }
   if (empty($args)) {

This is massively changing the meaning of $locale_exists. It's allowably to have a different module that implements the locale() function without locale module being enabled. That's a weird hack that is incredibly outdated, but if we have that feature we can't just lose it here.

Also:

+  if (!isset($locale_exits)) {
+++ b/core/includes/cache.incundefined
@@ -26,13 +26,12 @@
function cache($bin = 'cache') {
+  $cache_objects = &drupal_static(__FUNCTION__, array());

Might be a case for the drupal_static_fast pattern?

+++ b/core/includes/module.incundefined
@@ -60,14 +64,29 @@ function module_load_all($bootstrap = FALSE) {
+function module_list($type = 'module_enabled', array $fixed_list = NULL, $reset = FALSE) {
+  // This static is only used for $fixed_list. It must not be a drupal_static(),
+  // since any call to drupal_static_reset() in unit tests would cause an
+  // attempt to retrieve the list of modules from the database (which does not
+  // exist).
+  static $module_list;
+
+  if ($reset) {
+    $module_list = NULL;
+    // Do nothing if no $type and no $fixed_list have been passed.
+    if (!isset($type) && !isset($fixed_list)) {
+      return;
+    }

We just got rid of this in #1503224: Cleanup module_list() :(

Gábor Hojtsy’s picture

While theoretically another module could implement locale() it was also possible in Drupal 7 to swap in the cache array with an arrayaccess implementation and skip most of the logic in locale() by feeding the data there. I have not looked at the updated cache backend for locale() but it is likely still swappable in some way now.

sun’s picture

Status:Needs work» Needs review
StatusFileSize
new4.33 KB
new22.29 KB
FAILED: [[SimpleTest]]: [MySQL] 41,291 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

The review issues on the t()/locale() change looked impossible to resolve for me, since we're calling into an API function without any kind of protection and check for whether that API is actually ready to operate, and it can't even be controlled through any kind of variable. But PHP code cannot be unloaded. So locale() is called unconditionally, just because it has been loaded at some point.

However. Since it seems like we need to retain that weird magic function "feature" as-is, and since we cannot unload PHP code, I simply turned it around, and put the the check into the locale() function. This means that the locale() function needs to make sure on its own that it is being called in an environment in which the (Locale) module is actually enabled (and thus its database tables are installed).

Use advanced drupal_static() pattern for cache().
Moved Locale module installation check from t() into locale() itself.

Status:Needs review» Needs work

The last submitted patch, test.setup_.107.patch, failed testing.

sun’s picture

Status:Needs work» Needs review
StatusFileSize
new1.05 KB
new22.27 KB
PASSED: [[SimpleTest]]: [MySQL] 41,294 pass(es).
[ View ]

Fixed locale_reset() relies on 'locale' drupal_static name.

sun’s picture

I think the latest patch addresses the concerns of #105 as much and as cleanly as possible.

Alternative "implementations" of locale() will only have to follow those changes, if they are used in tests. (or more precisely: if the parent-site/test-runner has the alternative implementation loaded and intends to run tests)

The only part I did not address yet is:

We just got rid of this in #1503224: Cleanup module_list() :(

Yes. It's pretty clear to me by now that the removal of $reset arguments from module_list() & co was wrong, since it allows a potentially fixed module list to get (unintentionally) reset, which not only affects tests but also special front controllers like the installer and update.php. However, there's hope that #1331486: Move module_invoke_*() and friends to an Extensions class will allow us to replace the entire module/extensions service with a specialized LockedExtensions implementation, so we'd get rid of $reset arguments there and all of this could be simplified later on.

sun’s picture

Status:Needs review» Reviewed & tested by the community

Tentatively moving back to RTBC, since the only concerns since last RTBC have been addressed.

lucascaro’s picture

+1 to RTBC

Gábor Hojtsy’s picture

@catch says the cache clearing techniques here might help with solving #1787520: Translations not imported on installation as well which is also about a module being enabled but the fixed module list kept intact and therefore locale not exposing its stream wrapper which would be needed for importing .po files in the installer.

catch’s picture

Status:Reviewed & tested by the community» Needs work
+++ b/core/modules/locale/locale.moduleundefined
@@ -254,10 +254,22 @@ function locale($string = NULL, $context = NULL, $langcode = NULL) {
+  if (!isset($locale_exits)) {
+    $locale_exists = function_exists('module_exists') && module_exists('locale');

Still locale_exits.

sun’s picture

Status:Needs work» Needs review
StatusFileSize
new642 bytes
new22.27 KB
PASSED: [[SimpleTest]]: [MySQL] 41,470 pass(es).
[ View ]

oh. Sorry, that detail wasn't clear in the original review.

lucascaro’s picture

Status:Needs review» Reviewed & tested by the community

Just in case I've re tested installing using drush si and interactively using the browser. It all seemed to be working fine both before and after applying the patch. I've also ran the tests using the UI (the only other thing that is not tested by the bot, I think) and it works.

catch’s picture

Status:Reviewed & tested by the community» Fixed

Thanks! Committed/pushed to 8.x. Bit of luck this will unblock some other issues.

lucascaro’s picture

awesome! is this worth backporting to D7? I started working on that a while ago but there have been some changes along the way, and @sun mentioned it could be tricky... I'd like to try anyways unless you guys think it can't or shouldn't be done.

chx’s picture

Priority:Normal» Critical
Status:Fixed» Reviewed & tested by the community
StatusFileSize
new529 bytes
PASSED: [[SimpleTest]]: [MySQL] 41,486 pass(es).
[ View ]

Opsie. Let me quote: http://php.net/unset

To unset() a global variable inside of a function, then use the $GLOBALS array to do so

tim.plunkett’s picture

RTBC +1, just for process's sake.

xjm’s picture

I'd ah... hesitate to backport this.

webchick’s picture

Priority:Critical» Normal
Status:Reviewed & tested by the community» Fixed

Committed and pushed the follow-up in #119.

For those to whom #119 didn't make sense, here's what chx had to say when I asked him about the patch:

"webchick: when it finishes install it doesnt unset the install_state so the system thinks it's still installing
webchick: for example, drupal_get_profile is broken (at least)"

chx’s picture

OK, so to follow up on how I found this: after a test method is torn down, drupal_get_profile still reports the testing profile because install_state lingers. Any other code that peeks at install_state would be similarly broken. And it's particularly hard to write a test for something that only happens outside of a test...

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

Anonymous’s picture

Issue summary:View changes

Updated issue summary.