Problem

Goal

  1. Eliminate the entirety of Simpletest hacks and overrides. Replace them with native multi-site functionality.
  2. Each web test gets an actual site in /sites/simpletest/$prefix.
  3. For WebTests, we simply let the installer do its job as usual.
  4. The site and test environment is negotiated only once, by conf_path(), via drupal_valid_test_ua().
  5. drupal_valid_test_ua() is adjusted to be able to operate without having access to settings.php, so as to remove a circular dependency in the early installer (install_begin_request()).

Anatomy

  1. TestBase::prepareDatabasePrefix() additionally prepares a site directory: /sites/simpletest/$prefix
  2. TestBase::setUp() (new) primes drupal_valid_test_ua() with the test prefix and resets conf_path().
  3. conf_path() uses drupal_valid_test_ua() to force-override the site directory to /sites/simpletest/$prefix
  4. WebTestBase::setUp() executes the non-interactive installer (install_drupal()) like now, but instead of futzing with any environment data, the installer simply does its job → Drupal is installed into the test site directory with an own settings.php file.
  5. To allow conf_path() to use drupal_valid_test_ua() in super-early bootstrap, we no longer rely on $drupal_hash_salt in settings.php, but instead, a test-prefix-specific $private_key is read from a new /sites/simpletest/$prefix/key.php file (written by drupal_generate_test_ua()).
  6. Due to the nested site subdirectory (simpletest/$prefix), a test site directory is not a valid regular site directory, and thus not discovered by find_conf_path() at regular runtime.

As a result, each web test operates in a clean and own site environment:

sites/simpletest/$prefix
sites/simpletest/$prefix/files
sites/simpletest/$prefix/private
sites/simpletest/$prefix/temp
sites/simpletest/$prefix/settings.php

Security

  1. The site discovery process (find_conf_path()) does not support nested subdirectories. The /sites/[simpletest]/12345 directories are not considered at regular runtime.
  2. The /sites/simpletest directory itself is not discovered, because it does not map to any valid hostname.

    (That is, unless you would locally install on a http://simpletest hostname, but that edge-case is intentionally ignored.)

  3. Each executed test (method) generates a new private key that is stored in a /sites/simpletest/$prefix/.htkey file specific to the test run/prefix, using the same cryptographic algorithm as before.
  4. Each inbound HTTP request to Drupal
    1. checks whether User-Agent HTTP header contains the simpletest prefix
    2. if it does, the private key is read from the corresponding /sites/simpletest/$prefix/.htkey file in the filesystem (hard-wired)
    3. only if the private key can be read
    4. only if the given untrusted HMAC can be validated using the private key
    5. then the User-Agent is valid and conf_path() skips its normal logic and force-routes the client into the test site directory and environment.

    No cryptographic change compared to HEAD. The only change: The private key is no longer read from the $drupal_hash_salt variable in settings.php, but from a custom private key file instead, because it is required before settings.php is loaded. In fact, a successful HMAC validation controls which settings.php is going to be loaded for a test (in super-early bootstrap).

  5. The /sites/simpletest directory has to be writable. The extent of "writable" varies by whether tests are executed via the run-tests.sh command line (only) or the Simpletest UI.
  6. simpletest_requirements() ensures to write a /sites/simpletest/.htaccess file (same as for public files directory), in order to prevent any contained PHP files in any of the test site directories from being executed via public HTTP requests.
  7. Each WebTestBase operates in a completely ordinary Drupal [test] site directory/environment. This means that /sites/simpletest/12345/settings.php does exist and it does contain the database credentials of the parent site (plus prefix).
    (That is, unless WebTestBase is customized to work with custom database connection info [which I'd like to make possible later])

    This is deemed to be acceptable because (1) the settings.php file is not publicly accessible and (2) it is not recommended to run tests on production sites in the first place.

  8. To truly leverage the new test site environment mechanism and harden our Drupal core test coverage, the interactive installer tests are submitting the database credentials of the parent site to the child site in a HTTP POST request on the third installer screen. This is deemed to be acceptable, because the origin and target host of the HTTP request is identical; i.e., the POST data is transferred internally via HTTP on the same host only and it is not leaving the private network.

API changes

  1. Developers that want to run tests need to create a /sites/simpletest directory and make it writable.
  2. None otherwise.

Dependencies

  1. #2170023: Use exceptions when something goes wrong in test setup
  2. #1376122: Stream wrappers of parent site are leaking into all tests
  3. #2176141: Add a return value to file_save_htaccess()
  4. #2176131: Database configuration form in installer still uses 'db_prefix' instead of 'prefix'
  5. #2176151: rebuild.php does not include /core/vendor/autoload.php

Blocks

  1. #1881582: Change configuration overrides to use $config instead of $conf
  2. #1757536: Move settings.php to /settings directory, fold sites.php into settings.php
CommentFileSizeAuthor
#114 2171683.114.patch102.31 KBalexpott
#114 interdiff.txt655 bytesalexpott
#112 interdiff.txt3.11 KBsun
#112 test.site_.112.patch104.5 KBsun
#111 interdiff.txt2.21 KBsun
#111 test.site_.110.patch103.53 KBsun
#108 104-108-interdiff.txt1001 bytesalexpott
#108 2171683.108.patch102.83 KBalexpott
#104 interdiff.txt7.38 KBsun
#104 test.site_.104.patch101.85 KBsun
#100 test.site_.100.patch102.62 KBsun
#99 test.site_.99.patch102.62 KBsun
#96 test.site_.96.patch102.62 KBsun
#92 test.site_.92.patch102.66 KBsun
#90 test.site_.81.patch102.66 KBtstoeckler
#86 Screenshot 2014-01-30 07.51.19.png37.39 KBlarowlan
#86 Screenshot 2014-01-30 07.33.38.png62.98 KBlarowlan
#81 test.site_.81.patch102.66 KBsun
#80 interdiff.txt15.04 KBsun
#80 test.site_.80.patch104.93 KBsun
#76 2171683-75-simpletest-multisite.patch104.71 KBtstoeckler
#64 interdiff.txt1.05 KBsun
#64 test.site_.64.patch118.95 KBsun
#63 interdiff.txt20.87 KBsun
#63 test.site_.63.patch119.71 KBsun
#61 interdiff.txt776 bytessun
#61 test.site_.61.patch136.03 KBsun
#60 interdiff.txt13.68 KBsun
#60 test.site_.60.patch136.79 KBsun
#48 interdiff.txt9.63 KBsun
#48 test.site_.47.patch141.36 KBsun
#45 interdiff.txt30 KBsun
#45 test.site_.45.patch140.08 KBsun
#40 interdiff.txt9.7 KBsun
#40 test.site_.40.patch125.77 KBsun
#37 interdiff.txt47.31 KBsun
#37 test.site_.37.patch116.69 KBsun
#34 interdiff.txt6.28 KBsun
#34 test.site_.34.patch88.44 KBsun
#31 interdiff.txt11.87 KBsun
#31 test.site_.31.patch88.21 KBsun
#26 interdiff.txt19.72 KBsun
#26 test.site_.26.patch83.03 KBsun
#23 interdiff.txt8.61 KBsun
#23 test.site_.23.patch73.43 KBsun
#17 test-site-2171683.17.patch69.87 KBlarowlan
#17 interdiff.txt1.98 KBlarowlan
#14 interdiff.txt795 bytessun
#14 test.site_.14.patch68.7 KBsun
#12 interdiff.txt33.49 KBsun
#12 test.site_.12.patch68.7 KBsun
#10 interdiff.txt699 bytessun
#10 test.site_.10.patch43.17 KBsun
#7 interdiff.txt1014 bytessun
#7 test.site_.7.patch43.06 KBsun
#1 test.site_.1.patch42.65 KBsun
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

FileSize
42.65 KB
amateescu’s picture

Developers that want to run tests need to create a /sites/simpletest directory and make it writable.

Can't we do this automatically when the simpletest module is installed?

larowlan’s picture

+1000000000

sun’s picture

Hm. Can't make sense of the test failures.

On one hand, PIFR prepares the (entire) checkout directory with:

[12:20:02] Command [chmod -R u+rw sites/default/files/checkout 2>&1] succeeded.

I.e., all directories should be writable. But yet:

file_put_contents(/var/lib/drupaltestbot/sites/default/files/checkout/sites/simpletest/84449/key.php): failed to open stream: Permission denied

file_put_contents('/var/lib/drupaltestbot/sites/default/files/checkout/sites/simpletest/84449/key.php', '<?php
$private_key = 'CDGUSUNeWqAwWZ9JHCmOIvdqwraOHDZd9VG7tftIpa4';
')
drupal_generate_test_ua('simpletest84449')
Drupal\simpletest\WebTestBase->curlInitialize()
Drupal\simpletest\WebTestBase->curlExec(Array)
Drupal\simpletest\WebTestBase->drupalGet('user', Array)
Drupal\simpletest\WebTestBase->drupalPostForm('user', Array, 'Log in')
Drupal\simpletest\WebTestBase->drupalLogin(Object)
Drupal\action\Tests\ActionUninstallTest->testActionUninstall()
Drupal\simpletest\TestBase->run()
simpletest_script_run_one_test('1', 'Drupal\action\Tests\ActionUninstallTest')

I'm not able to reproduce that permission error locally... (but then again, I'm on Windows)

sun’s picture

Status: Needs work » Needs review
FileSize
43.06 KB
1014 bytes

Fixed test site directory is no longer writable after executing the installer.

sun’s picture

FileSize
43.17 KB
699 bytes

Fixed static in drupal_generate_test_ua() causes no new key.php file to be created for subsequent test prefixes.

With this, all web tests should pass again.

The last submitted patch, 10: test.site_.10.patch, failed testing.

sun’s picture

FileSize
68.7 KB
33.49 KB

Major update + significant improvements:

  1. Write a proper settings.php for web test customizations (following instructions in INSTALL.txt).
  2. Fixed test (fatal) error.log file setup to use new test site directory.
  3. Fixed installer uses 'db_prefix' form value instead of 'prefix'.
  4. Fixed WebTestBase::installParameters() passes completely invalid form values for database settings to installer.
  5. Fixed system_requirements() uses wrong key syntax to check for filesystem $conf overrides in installer.
  6. Fixed file_save_htaccess() appends bogus leading slash to stream wrapper URI.
  7. Fixed WebTestBase::rebuildContainer() builds a new container instead of using the existing dumped in the child site.
  8. Fixed Some tests expect the private/temp paths to be UI-configurable.
  9. Fixed stream wrapper tests + unnecessary dfac() after executing installer.
  10. Fixed verbose test output does not account for request headers, nor multiple requests.
  11. Manually create private/temp file directories in WebTestBase::setUp() after no longer relying on installer.
  12. Clarify that the chmod() on the test site directory does not need to be reverted as tearDown() deletes it entirely.
  13. Protect drupal_valid_test_ua()'s key.php file inclusion against weak PHP error reporting settings on production sites.
  14. Automatically try to create the 'sites/simpletest' directory upon installation of Simpletest module.

These changes are not only simplifying tons of stuff, they also present a major performance improvement for web tests.

However, AggregatorCronTest & Co will still fail. The test invokes $this->cronRun(), which issues an HTTP request to the child site to run cron, and something extraordinarily weird in drupal_cron_run() appears to completely + entirely destroy BOTH the child and parent site environment, causing any subsequent HTTP requests to the child site to end up on the parent site — whereas that is technically impossible due to drupal_valid_test_ua().

I debugged for hours (and a few of the improvements are caused by heavy debugging), but for the life of me, I'm not able to find the root cause. The test succeeds (with expected failures) when injecting an early-return into drupal_cron_run(), so as to skip the entire function. → drupal_cron_run() definitely appears to be the cause, but after introspecting all hook_queue_info() + hook_cron() implementations in core, it does not appear to be the root cause.

The last submitted patch, 12: test.site_.12.patch, failed testing.

sun’s picture

FileSize
68.7 KB
795 bytes

Fixed invalid PHP syntax in simpletest.install.

chx’s picture

This makes sites that run tests vulnerable to the malicious upload script attack the phpstorage system is architectured to protect against. Is there a reason why we aren't using it for the key.php file?

The last submitted patch, 14: test.site_.14.patch, failed testing.

larowlan’s picture

FileSize
1.98 KB
69.87 KB

So the issue with cron is that Aggregator uses Guzzle to fetch stuff (and so does update module and numerous others I'd guess).
And we have Drupal\Core\Http\Plugin\SimpletestHttpRequestSubscriber::onBeforeSendRequest() to set the user-agent header using drupal_generate_test_ua().
Now because drupal_generate_test_ua() hasn't been called in the child site (only in the parent site) the $key and $last_prefix statics aren't set in that function, so it generates a new key.php so your child site disappears into the ether.

This adds a new static to track the ua from the child site, without the need to regenerate the key (that drupal_generate_test_ua does).

It's hacky, but it gets AggregatorCronTest passing locally.

Even more reason to have a proper testing kernel or event request subscriber in my opinion.

But baby steps.

Another interesting question is why is that subscriber on in core, all the time.
Surely it should live in simpletest.services.yml?

Status: Needs review » Needs work

The last submitted patch, 17: test-site-2171683.17.patch, failed testing.

Berdir’s picture

It needs to be on all the time because the child sites won't have simpletest.module installed but when they do requests to themself (like aggregator tests using node rss feeds), they need that user agent.

larowlan’s picture

@berdir Fair enough.

So no idea what testbot issue is.

Installs fine locally, and AggregatorCronTest passes from UI and run-tests.sh.

Going to requeue, if that doesn't help, putting it aside for today, already spent a couple of hours debugging.

larowlan’s picture

Status: Needs work » Needs review

17: test-site-2171683.17.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 17: test-site-2171683.17.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
73.43 KB
8.61 KB

@larowlan saved my day. Thanks a ton! :)

  1. Fixed TestBase::tearDown() assumes that (any) prefixed database tables would exist.
  2. Restored resetAll() after running installer; required to synchronize parent/child site environment.
  3. Fixed drupal_generate_test_ua() breaks test environment when called from within the child site. Thanks @larowlan! :)

The last submitted patch, 23: test.site_.23.patch, failed testing.

chx’s picture

This makes sites that run tests vulnerable to the malicious upload script attack the phpstorage system is architectured to protect against. Is there a reason why we aren't using it for the key.php file?

sun’s picture

FileSize
83.03 KB
19.72 KB
  1. Mark new _drupal_request_initialize() helper function as private.
  2. Clean up drupal_settings_initialize() and call it again after executing installer.
  3. Fixed bogus staging config directory assumptions in Field and Node config import tests.
  4. Fixed WebTestBase::refreshVariables() destroys global $conf from test site settings.php.
  5. Removed no longer necessary global variable protection from WebTestBase::writeSettings().
  6. Fixed WebTestBase::drupalGetHeaders() relies on $this->headers, so retain it as-is.
  7. Kill $GLOBALS['drupal_test_info']. Replaced by new DRUPAL_TEST_IN_CHILD_SITE constant.
  8. Removed extra database prefix safety net from _drupal_bootstrap_configuration(), which was necessary for early development only.

@chx: Sorry for not replying earlier to your question. I'm not able to see what PhpStorage would buy us aside from problems:

  1. PhpStorageFactory depends on Settings and drupal_hash_salt() → Circular dependency. If anything, PhpStorage only adds dependencies that are not yet available at the point in time when drupal_valid_test_ua() is called.
  2. MTimeProtected[Fast]FileStorage needs a secret key to operate → but what we are storing is the secret key itself.
  3. On production sites, (1) there is no /sites/simpletest directory and (2) the entire /sites directory is not writable → even if you find yourself in a position to be able to write arbitrary files, you will not be able to write the /sites/simpletest/12345/key.php file.

Hence: All you need to do to keep your production site secure is to ensure that the /sites directory is not writable. The /sites directory was not writable before, and there's no reason for why you should make it writable in D8.

People should not run tests on a production site. That request should unilaterally be answered with:

305 Use Proxy
Location: https://drupal.org/project/bad_judgement

The last submitted patch, 26: test.site_.26.patch, failed testing.

The last submitted patch, 26: test.site_.26.patch, failed testing.

The last submitted patch, 26: test.site_.26.patch, failed testing.

chx’s picture

While it is not recommended to run simpletest on production sites, this patch makes it fundamentally insecure to do so. That's not a good idea. Now that I read more of the patch I see
$key_file = "<?php\n\$private_key = '$private_key';\n";

this being the key.php. Why on earth is this a PHP file instead of simply storing a string and thus avoiding this whole conundrum? Also, if sites is made writeable it needs to contain the usual htaccess we place in writeable directories to avoid Apache serving PHP files from it.

sun’s picture

FileSize
88.21 KB
11.87 KB
  1. Simplified conditional test child site logic in drupal_generate_test_ua().
  2. Added custom error/exception handlers for run-tests.sh to debug early script errors.
  3. Fixed critical logic error: 'run-tests.sh --clean' recursed into parent directories, deleting the entire sites directory.
  4. Fixed simpletest_install() throws a warning message even if the 'sites/simpletest' directory exists already.
  5. Use a hidden + plain-text .htkey file instead of key.php.
  6. Write a simpletest prefix specific .htaccess file to the test site directory.

@chx:

While it is not recommended to run simpletest on production sites, this patch makes it fundamentally insecure to do so.

I guess I don't see what the problem is. If you are able to identify a security issue with this patch (that doesn't exist in HEAD already), then I'd like to learn.

But to address your concerns (hopefully to a satisfying extent), (1) I've changed the private key file into a hidden plain-text .htkey file and (2) changed drupal_generate_test_ua() to additionally create a .htaccess file that is specific to the simpletest prefix.

The last submitted patch, 31: test.site_.31.patch, failed testing.

chx’s picture

> I guess I don't see what the problem is. If you are able to identify a security issue with this patch (that doesn't exist in HEAD already), then I'd like to learn.

There were two, both of them relying on an upload script lying around which takes the filename from $_GET. This is not theory, mind you, such scripts are found in the wild and played crucial roles in some high profile site breaches. So if such a script is present then a) an attacker could've written the previous key.php and get it included by Drupal -- arbitrary script run b) just upload any PHP to a writeable dir (nee sites) again arbitrary script run.

Drupal combats b) with the htaccess written in the files directory removing PHP running completely. Drupal 8 combats a) with the phpstorage mechanism.

This patch now protects from a) by not including PHP files so that's a done. b) is partially protected because you still can write the sites/simpletest directory and browse to there -- only sites/simpletest/$secret is protected. I think reusing the htaccess we use for the files directory would work, wouldn't it? Is there any PHP file you want to browse to inside the sites/simpletest directory?

sun’s picture

FileSize
88.44 KB
6.28 KB
  1. Removed obsolete comment about unconditionally including private key file.
  2. Fixed permissions on sites/simpletest directory created by simpletest_install().
  3. Temporarily disabled .htaccess protection to focus on primary test failures first.
  4. Ensure that DRUPAL_TEST_IN_CHILD_SITE constant is only defined by _drupal_bootstrap_configuration().
  5. Fixed WebTestBase::writeSettings() is unable to rewrite settings.php whenever system_requirements() has run.

The last submitted patch, 34: test.site_.34.patch, failed testing.

The last submitted patch, 34: test.site_.34.patch, failed testing.

sun’s picture

FileSize
116.69 KB
47.31 KB
  1. Allow me to properly inspect + debug failing tests by retaining test artifacts through a new clean.artifacts configuration setting.
  2. Verbosely output all request/response headers in RESTTestBase::httpRequest().
  3. Refactor TestBase::run() to require TestBase::prepareEnvironment() to set up test site directory and make setUp() less fragile.
  4. Refactor InstallerTest for new test site architecture and allow InstallerTranslationTest to extend it properly.
  5. Fixed errors and exceptions thrown in ErrorHandlerTest are finally picked up by test runner - ignore them.
  6. Same goes for ShutdownFunctionTest.
  7. Fixed unit tests fail to create directories, because drupal_mkdir() does not properly check for config.factory service.

Interactive installer tests, mail system tests, and error handler tests are passing again with this patch. And they're much more accurate than ever before. :)

That said, I'm able to reproduce the failures in LanguageNegotiationInfoTest locally, but I have no idea why they occur. That test does not appear to rely on any special test environment variables. Although it massively relies on global state... I hope that #1862202: Objectify the language system will fix that.

Lastly, please note that the new + additional .htaccess file protection is still commented out. My plan is to keep that disabled until the basic patch is green. That is, because various additional tests are failing with that protection, seemingly because they attempt to retrieve files from the public files directory of the test site via HTTP requests that do not use the Simpletest User-Agent header. (→ fun!)

The last submitted patch, 37: test.site_.37.patch, failed testing.

chx’s picture

There is absolutely no need for this newfangled htaccess protection if you use the file_save_htaccess() function as you clearly plan to. file_save_htaccess nukes PHP without denying access -- it is after what we use for public files directory and is enough here as well.

sun’s picture

FileSize
125.77 KB
9.7 KB
  1. Fixed DrupalKernelTest must use DUTB as it relies on configuration directories + globals + config_get_config_directory() (but skip everything else of DUTB).
  2. Fixed expected exceptions thrown in Ajax\FormValuesTest are actually caught and logged now.
  3. Fixed Fatal error: Call to a member function hasPath() on a non-object in ViewListController::getDisplayPaths().

This should leave us with LanguageNegotiationInfoTest + MenuRouterTest... (before adding back any security protections)

chx’s picture

There's no need for "any" there's only need for one and that one won't interfere with your tests in any way or form...

The last submitted patch, 40: test.site_.40.patch, failed testing.

tstoeckler’s picture

Review of the interdiff from #37:

  1. +++ b/core/includes/file.inc
    @@ -1496,7 +1496,8 @@ function drupal_mkdir($uri, $mode = NULL, $recursive = FALSE, $context = NULL) {
         // During early update there's no container.
    ...
    +    if (is_object($container) && $container->has('config.factory')) {
    ...
    +    $container = \Drupal::getContainer();
    ...
    -    if (is_object(\Drupal::getContainer())) {
    

    We should update the comment to say that during testing there might be no 'config.factory' service.

  2. +++ b/core/modules/simpletest/config/simpletest.settings.yml
    @@ -1,3 +1,5 @@
     clear_results: '1'
    ...
    +  artifacts: true
    ...
    +clean:
    

    I think having both 'clean' and 'clear' is confusing. Is the plan to merge them? That said, I actually think 'clear' is more accurate. You're cleaning the system of artifacts, you're not actually cleaning the artifacts...

  3. +++ b/core/modules/simpletest/lib/Drupal/simpletest/Form/SimpletestSettingsForm.php
    @@ -30,18 +30,24 @@ public function buildForm(array $form, array &$form_state) {
    +      '#description' => $this->t('Retains the test site directory and database tables of all executed tests instead of deleting them to allow for advanced debugging.'),
    

    Yay, this is *sooo* useful. +10000

  4. +++ b/core/modules/simpletest/lib/Drupal/simpletest/TestBase.php
    @@ -791,19 +792,34 @@ public function run(array $methods = array()) {
    +            break;
    ...
    +            // parent site and thus must be aborted.
    ...
    +            // directory. If it fails, a test would possibly operate in the
    ...
    +            // Drupal site by creating a random database prefix and test site
    ...
    +            // The prepareEnvironment() method isolates the test from the parent
    

    The comment should also say that we're aborting the entire process and skip all future tests as well. Otherwise this should be continue; not break;

  5. +++ b/core/modules/simpletest/lib/Drupal/simpletest/TestBase.php
    @@ -791,19 +792,34 @@ public function run(array $methods = array()) {
               }
    ...
    +            $this->exceptionHandler($e);
    ...
    +          catch (\Exception $e) {
    ...
    +          }
    ...
    +            $this->tearDown();
    ...
    +            $this->$method();
    ...
    +          try {
    ...
               }
    ...
    +            break;
    ...
    +            // But ensure to clean up and restore the environment, since
    ...
    +            $this->exceptionHandler($e);
    ...
    +          catch (\Exception $e) {
    ...
    +            $this->setUp();
    ...
    +          try {
    ...
    +          }
    ...
    +            // Abort if setUp() fails, since all test methods will fail.
    ...
    +            // prepareEnvironment() succeeded.
                 $this->tearDown();
    

    Not sure that the code flow is correct here:
    With this if an exception is being thrown in the test method tearDown() is not called. Instead I think it would make sense to put setUp() and $method() in one try {} block and then call tearDown() in the catch {}.

  6. +++ b/core/modules/simpletest/lib/Drupal/simpletest/TestBase.php
    @@ -891,16 +911,26 @@ protected function changeDatabasePrefix() {
    +   * This method is final as it must not be overridden by tests.
    ...
    +  protected final function prepareEnvironment() {
    

    This is *awesome* but we should explain why, i.e. that you can kill your site if you override this incorrectly.

tstoeckler’s picture

Review of the interdiff in #40:

  1. +++ b/core/modules/system/lib/Drupal/system/Tests/Ajax/FormValuesTest.php
    @@ -53,6 +53,9 @@ function testSimpleAjaxFormValue() {
    +    $this->assertFalse(file_exists($log_file) && file_get_contents($log_file), 'error.log is empty.');
    

    This is a strange assertion. It will pass if error.log does not exist, is that intentional? If so the comment above needs to explain why that can happen.

  2. +++ b/core/modules/system/lib/Drupal/system/Tests/DrupalKernel/DrupalKernelTest.php
    @@ -26,16 +26,9 @@ public static function getInfo() {
    +    // and everything else in DrupalUnitTestBase::setUp().
    ...
    +    // Only create configuration directories, but skip unit testing DrupalKernel
    

    "skip unit testing DrupalKernel" I thought that's what the thing is doing? I don't understand that commment.

  3. +++ b/core/modules/views_ui/lib/Drupal/views_ui/ViewListController.php
    @@ -258,6 +258,10 @@ protected function getDisplayPaths(EntityInterface $view) {
    +      }
    ...
    +        continue;
    ...
    +      if (!isset($display)) {
    ...
    +      // DisplayBag may return NULL if a plugin was not found.
    ...
         foreach ($executable->displayHandlers as $display) {
    

    Minor: Maybe array_filter() is more concise/clear?

sun’s picture

FileSize
140.08 KB
30 KB
  1. Fixed WebTestBase::writeCustomTranslations() does not correctly write an empty translations array to settings.php. (fixing MenuRouterTest)
  2. Changed visibility of TestBase::prepareEnvironment() into private to guarantee it is invoked only once by TestBase itself.
  3. Merged 8.x to pick up #1862202: Objectify the language system — yay! :)
  4. Fixed LanguageNegotiationInfoTest does not properly synchronize child/parent environments + uses weird custom code to install test module.
  5. Improved comments and ensure that TestBase::run() invokes tearDown() if test method throws an exception.
  6. Renamed clean.artifacts into clear.artifacts.
  7. Added TestBase::assertNoErrorsLogged() helper method for tests throwing expected errors.
  8. Fixed --keep-artifacts is not forwarded to spawned run-tests.sh processes.

This patch is supposed to come back green. In case it does, then I'll implement the (single) .htaccess file requested by @chx & we'll be done. :)

Sorry for the patch-serial interdiff — I had to merge 8.x into my branch in the middle.

The last submitted patch, 45: test.site_.45.patch, failed testing.

tstoeckler’s picture

  1. +++ b/core/modules/language/lib/Drupal/language/Tests/LanguageNegotiationInfoTest.php
    @@ -46,31 +39,62 @@ public static function getInfo() {
    +    $this->container->get('state')->setMultiple($values);
    ...
    +    return $this->container->get('language_manager');
    ...
    +    $this->container->get('plugin.manager.language_negotiation_method')->clearCachedDefinitions();
    ...
    +    $this->container->get('module_handler')->install(array('language_test'));
    
    @@ -118,10 +152,11 @@ function testInfoAlterations() {
    +    $this->container->get('module_handler')->uninstall(array('language_test'));
    

    The usage of $this->container in tests is discouraged. \Drupal should be used instead.

  2. +++ b/core/modules/simpletest/config/simpletest.settings.yml
    @@ -1,5 +1,6 @@
    +# @todo Move into 'clear'.
    ...
    +clear:
    

    Yeah, thanks :-)

sun’s picture

Issue summary: View changes
Issue tags: +API clean-up
FileSize
141.36 KB
9.63 KB
  1. Fixed bogus condition in assertNoErrorsLogged() and expected exception handling in failing tests.
  2. Secure test sites directory with a single /sites/simpletest/.htaccess.

Mission accomplished. :-)

Added a Security chapter to the issue summary.

Thanks for your advice, @chx. I hope the added chapter accurately summarizes the changes. In case it does not, please let me know where I'm wrong (or feel free to edit it directly).

sun’s picture

Sorry, cross-posted with @tstoeckler...

The usage of $this->container in tests is discouraged. \Drupal should be used instead.

Tests are using both right now. And while I've recently heard people complaining about containers in tests, I do not think there is a real or formal agreement on "$this->container is discouraged" (yet).

In fact, I'd strongly argue that \Drupal is the much bigger curse of both: $this->container is a clear and cleanly manageable scope, as opposed to a completely uncontrolled static global \Drupal state that can literally be altered by anyone at any time. Second, our goal is to get rid of \Drupal entirely in the long run, so even just on strategical grounds only, that would be a move in the utterly wrong direction.

However, that discussion belongs into a different issue (and not here).

That said, this patch might help to eliminate some of the concerns and complaints about container usage in tests, because one of the primary root causes is that HEAD is currently operating with different environments between the child site and test runner. As a direct consequence, differences in state and content shouldn't actually be a surprise to anyone.

This patch not only removes all of the artificially enforced container differences between the child site and test runner (which is fine now, because we're operating in a cleanly separated site environment), it also goes one step further and changes WebTestBase::rebuildContainer() to essentially just reload the container that has been dumped in the child site already.

In any case, once this patch is in, there is plenty of room for improvements on the test container topic.

chx’s picture

This does look good to me. Thanks for keeping Drupal secure even in non recommended scenarios.

sun’s picture

Issue summary: View changes

Sorry, I missed two further crucial aspects in the Security chapter. For the sake of completeness and clarity, I'd like to ensure that all aspects are known and do not have to be discovered manually by anyone concerned:

  1. Each WebTestBase operates in a completely ordinary Drupal [test] site directory/environment. This means that /sites/simpletest/12345/settings.php does exist and it does contain the database credentials of the parent site (plus prefix).
    (That is, unless WebTestBase is customized to work with custom database connection info [which I'd like to make possible later])

    This is deemed to be acceptable because (1) the settings.php file is not publicly accessible and (2) it is not recommended to run tests on production sites in the first place.

  2. To truly leverage the new test site environment mechanism and harden our Drupal core test coverage, the interactive installer tests are submitting the database credentials of the parent site to the child site in a HTTP POST request on the third installer screen. This is deemed to be acceptable, because the origin and target host of the HTTP request is identical; i.e., the POST data is transferred internally via HTTP on the same host only and it is not leaving the private network.

Also added to the issue summary.

larowlan’s picture

Each WebTestBase operates in a completely ordinary Drupal [test] site directory/environment. This means that /sites/simpletest/12345/settings.php does exist and it does contain the database credentials of the parent site (plus prefix).
(That is, unless WebTestBase is customized to work with custom database connection info [which I'd like to make possible later])

Will definitely use this in our build process by configuring it to use SQLite in /dev/shm

sun’s picture

(That is, unless WebTestBase is customized to work with custom database connection info [which I'd like to make possible later])

Will definitely use this in our build process by configuring it to use SQLite in /dev/shm

I have some (old) code almost ready for this. In fact, now that I recall, it's even available online:

#1808220: Remove run-tests.sh dependency on existing/installed parent site

The gist: In your settings.php:

$databases['simpletest']['default'] = array(...);

Formerly blocked on exactly the issues that are being fixed by this patch. After this patch here went in, the patch over there will be ~10 LoC. → Profit! :-)

tstoeckler’s picture

So because I don't think 140 KB patches are very reviewable I would like to extract the improvements to the test setup code into #2170023: Use exceptions when something goes wrong in test setup where I already pursued a similar approach already - my approach was just not as refined as the one here. I would only spend time on that, however, if people actually agree with that approach. I.e. if this goes to RTBC and then commit within the next week then that time would not be well spent. So, what do you folks think?

sun’s picture

Ready for final reviews

I intensively reviewed this patch a couple of times myself already and believe it is ready to fly.

I don't want to down-play the size/amount of changes, but they are almost exclusively refactoring code and logic under the hood only, in order to make it compatible and comply with the new test site directory/environment. There are no public API changes involved.

I additionally informed the security team about this change proposal to check whether they can spot any problems. The Security chapter in the issue summary hopefully helps to speed up security reviews.

The changes of this patch are a hard blocker for #1757536: Move settings.php to /settings directory, fold sites.php into settings.php, which I really want to get done before beta and will immediately get back to and proceed with as soon as this change is in.

larowlan’s picture

Firstly, this is a great step towards untangling the spaghetti independencies between the installer, simpletest and the kernel and is definitely a step towards Kernel based testing.
Also it catches a few bugs/phony passes in HEAD that we have test coverage for but are only passing because of the Simpletest overrides, ie our testing with this patch better matches the reality of a live site - so ++ again.
Most of these points are minor.

  1. +++ b/core/includes/bootstrap.inc
    @@ -563,20 +519,33 @@ function drupal_valid_http_host($host) {
    + * @todo D8: Eliminate this entirely in favor of Request object.
    

    sneaky: can we reference http://drupal.org/2016629 with this @todo.

  2. +++ b/core/lib/Drupal/Core/StreamWrapper/PublicStream.php
    @@ -38,13 +38,6 @@ public function getExternalUrl() {
    -    if ($test_prefix = drupal_valid_test_ua()) {
    -      // Append the testing suffix unless already given.
    -      // @see \Drupal\simpletest\WebTestBase::setUp()
    -      if (strpos($base_path, '/simpletest/' . substr($test_prefix, 10)) === FALSE) {
    -        return $base_path . '/simpletest/' . substr($test_prefix, 10);
    -      }
    -    }
    

    This is nice cleanup, production code shouldn't need to be test aware, ideally - so nice.

  3. +++ b/core/lib/Drupal/Core/SystemListingInfo.php
    @@ -28,6 +28,7 @@ protected function profiles($directory) {
    +    // @todo !drupal_installation_attempted() defeats the whole purpose?
    

    Is there a url for this @todo?

  4. +++ b/core/modules/language/lib/Drupal/language/Tests/LanguageNegotiationInfoTest.php
    @@ -87,30 +111,40 @@ function testInfoAlterations() {
    +    //$negotiation = variable_get("language_negotiation_$type", array());
    

    Can we get a url to the issue which will reinstate this, makes it easier to grep for relevant todos.

  5. +++ b/core/modules/language/lib/Drupal/language/Tests/LanguageNegotiationInfoTest.php
    @@ -87,30 +111,40 @@ function testInfoAlterations() {
    +    $this->assertNoFieldByName($form_field, NULL, 'Interface language negotiation method unavailable.');
    ...
    -    $this->assertNoFieldByXPath("//input[@name=\"$form_field\"]", NULL, 'Interface language negotiation method unavailable.');
    ...
    -        $this->assertFieldByXPath("//input[@name=\"$form_field\"]", NULL, format_string('Type-specific test language negotiation method available for %type.', array('%type' => $type)));
    +        $this->assertFieldByName($form_field, NULL, format_string('Type-specific test language negotiation method available for %type.', array('%type' => $type)));
    ...
    -        $this->assertNoFieldByXPath("//input[@name=\"$form_field\"]", NULL, format_string('Type-specific test language negotiation method unavailable for %type.', array('%type' => $type)));
    +        $this->assertNoFieldByName($form_field, NULL, format_string('Type-specific test language negotiation method unavailable for %type.', array('%type' => $type)));
    

    nice cleanup

  6. +++ b/core/modules/simpletest/config/simpletest.settings.yml
    @@ -1,3 +1,6 @@
    +# @todo Move into 'clear'.
    

    this can go now?

  7. +++ b/core/modules/simpletest/lib/Drupal/simpletest/Form/SimpletestSettingsForm.php
    @@ -30,18 +30,24 @@ public function buildForm(array $form, array &$form_state) {
    +    $form['general']['simpletest_clear_artifacts'] = array(
    +      '#type' => 'checkbox',
    +      '#title' => $this->t('Keep test artifacts'),
    +      '#description' => $this->t('Retains the test site directory and database tables of all executed tests instead of deleting them to allow for advanced debugging.'),
    +      '#default_value' => !$config->get('clear.artifacts'),
    +    );
    

    this is a very useful addition

  8. +++ b/core/modules/simpletest/lib/Drupal/simpletest/TestBase.php
    @@ -857,34 +911,25 @@ protected function prepareDatabasePrefix() {
    -      // If $this->prepareDatabasePrefix() failed to work, return without
    -      // setting $this->setupDatabasePrefix to TRUE, so setUp() methods will
    -      // know to bail out.
    -      if (empty($this->databasePrefix)) {
    -        return;
    -      }
    

    I think this is still useful, but instead of return, another RuntimeException?

  9. +++ b/core/modules/simpletest/lib/Drupal/simpletest/TestBase.php
    @@ -1088,30 +1122,31 @@ protected function tearDown() {
    +    // @todo Move into TestBase::run().
    

    can we get an issue link here

  10. +++ b/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.php
    @@ -764,56 +750,98 @@ protected function setUp() {
    +    // @todo While private/temporary filesystem paths can be preset in the
    ...
    +    // @todo Some mail system specific tests expect to be able to override the
    

    and again, issue link for @todos
    Pretty sure @catch already created one for the private files path.

  11. +++ b/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.php
    @@ -1180,12 +1227,25 @@ protected function curlExec($curl_options, $redirect = FALSE) {
    +    $this->requestHeaders[$this->requestId] = curl_getinfo($this->curlHandle, CURLINFO_HEADER_OUT);
    +
    +    // Sort response headers alphabetically, but keep the HTTP status first.
    +    $response_headers = array_shift($this->responseHeaders[$this->requestId]);
    +    sort($this->responseHeaders[$this->requestId]);
    +    $this->responseHeaders[$this->requestId] = $response_headers . implode('', $this->responseHeaders[$this->requestId]);
    +
    +    $this->requestId++;
    +
    

    more debugging++

  12. +++ b/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.php
    @@ -1840,6 +1931,28 @@ protected function serializePostValues($post = array()) {
    +  protected function translatePostValues(array $values) {
    +    $edit = array();
    +    // The easiest and most straightforward way to translate values suitable for
    +    // WebTestBase::drupalPostForm() is to actually build the POST data string
    +    // and convert the resulting key/value pairs back into a flat array.
    +    $query = http_build_query($values);
    +    foreach (explode('&', $query) as $item) {
    +      list($key, $value) = explode('=', $item);
    +      $edit[urldecode($key)] = urldecode($value);
    +    }
    +    return $edit;
    +  }
    

    Is there anything already available in our vast codebase (including vendor) that already does this?

  13. +++ b/core/modules/simpletest/simpletest.api.php
    --- a/core/modules/simpletest/simpletest.install
    +++ b/core/modules/simpletest/simpletest.install
    

    Thoughts on making simpletest hidden here - to avoid 'what does this do' accidental enabling on production sites. aka Experts only. Would be an API change.

  14. +++ b/core/modules/system/lib/Drupal/system/Tests/DrupalKernel/DrupalKernelTest.php
    @@ -26,16 +26,12 @@ public static function getInfo() {
    +    // DrupalKernel relies on global $config_directories and requires those
    

    something we should tackle in the bootstrap cleanup - moving from globals in settings.php to the kernel and then dropping.

  15. +++ b/core/modules/system/lib/Drupal/system/Tests/Installer/InstallerTranslationTest.php
    @@ -15,6 +14,8 @@
    +  protected $langcode = 'de';
    

    nitpick: Needs docblock

  16. +++ b/core/modules/system/lib/Drupal/system/Tests/Installer/InstallerTranslationTest.php
    @@ -23,125 +24,34 @@ public static function getInfo() {
    +    // @todo Instead of actually downloading random translations that cannot be
    

    Can we get a link here too?

  17. +++ b/core/modules/system/lib/Drupal/system/Tests/InstallerTest.php
    @@ -8,13 +8,67 @@
    + * @todo Move majority of code into new Drupal\simpletest\InstallerTestBase.
    

    one more todo needing a link (committers seem to be asking for these now)

  18. +++ b/core/modules/system/lib/Drupal/system/Tests/InstallerTest.php
    @@ -24,132 +78,126 @@ public static function getInfo() {
    +  protected function setUpLanguage() {
    ...
    +  protected function setUpProfile() {
    ...
    +  protected function setUpSettings() {
    ...
    +  protected function setUpSite() {
    ...
    +  protected function setUpConfirm() {
    

    Need docblocks

  19. +++ b/core/modules/system/lib/Drupal/system/Tests/InstallerTest.php
    @@ -24,132 +78,126 @@ public static function getInfo() {
    +    $this->assertText($this->root_user->getUsername());
    

    So this would be a test for #2108623: Installing via the UI no longer logs in user 1 every time? yet this patch is green? Although I still have the feeling that is profile specific. Although the behaviour there is similar to the earlier issues, ie drupal_cron_run was busting it, so perhaps it might indeed be fixed, which would be bizarre, but a further indication that our current code knows too much about test overrides.

  20. +++ b/core/modules/views/lib/Drupal/views/Tests/Plugin/DisplayFeedTest.php
    @@ -19,7 +19,7 @@ class DisplayFeedTest extends PluginTestBase {
    -  public static $testViews = array('test_feed_display');
    +  public static $testViews = array('test_display_feed');
    

    how did this pass in head :)

  21. +++ b/core/modules/views_ui/lib/Drupal/views_ui/ViewListController.php
    @@ -258,6 +258,10 @@ protected function getDisplayPaths(EntityInterface $view) {
    +      // DisplayBag may return NULL if a plugin was not found.
    

    sounds like a missing module dependency in a test? ie a test view uses a plugin from a module that isn't installed?

  22. +++ b/core/rebuild.php
    @@ -15,6 +15,7 @@
    +require_once __DIR__ . '/vendor/autoload.php';
    

    unrelated?

sun’s picture

Thanks for the extensive review, @larowlan!

  1. re: adding a @see to #2016629: Refactor bootstrap to better utilize the kernel - I think the @todo is sufficient. For base system refactoring topics, it is not always safe to reference a particular issue, since a different issue might fix it, quite potentially leaving lots of stray + obsolete issue references behind throughout the code base.

    (And just to underline that, you and me just happened to discuss that it would be a good idea to split the initial Request handling to Drupal's bootstrap from aforementioned into a separate issue, since that topic on its own is far-reaching enough already :))

    In general, I'm not a fan of issue URL references in code. An issue tracker is detached from the code. Issues are managed independently from code. Proposals, opinions, and entire world perspectives can change in no time, causing a particular issue to become obsolete at any time.

    A @todo comment should clearly state and encompass the gist of a particular task that is foreseen already. Instead of making the code link to issues, the issue tracker should auto-generate issues for @todos in code. That is the only reliable solution.

    Otherwise, the result is only going to be a code base that is littered with stale issue references that will never get resolved. Clean-up attempts like #1813760: [meta] Clean up @todos and deprecated code would become substantially worse.

  2. Exactly. :)
  3. See #2175391: ExtensionDiscovery::getProfileDirectories() explicitly skips parent installation profile in installer, defeating its purpose
  4. That appears to be #2102477: Convert remainder of language negotiation settings to configuration system, but then again, I'm not sure whether that isn't a straight CMI conversion issue...
  5. :)
  6. See #2175151: Rename simpletest.settings:clear_results into clear.results
  7. :)
  8. Not applicable; You commented on removed code ;)
  9. See #2175397: Move test environment clearing from TestBase::tearDown() into TestBase::run()
  10. re: WebTestBase::setUp() @todos on (not) priming private/temp files directories + mail system in settings.php:

    Actually, I'm no longer sure whether those @todos are needed. In the end, the conclusion is that we are not able to hard-wire that configuration in settings.php, because some tests expect to be able to swap out the implementations in web tests.

    Of course, a different question is whether those tests are legit web tests in the first place and whether they shouldn't actually be DUTB or perhaps even unit tests. Especially the case of not enforcing the test mail collector in web tests feels a bit icky for me. I'm developing on Windows, so a malformed test isn't able to actually send out any e-mails. But if was using a system where that is possible, I'd be deeply concerned.

    If you agree, I can create a follow-up issue to discuss the ultimate fate of those @todos, but I do not think their addition should hold up this patch, since all they do is to document Lesser Known Facts™.

  11. :)
  12. re: WebTestBase::translatePostValues():

    Is there anything already available in our vast codebase (including vendor) that already does this?

    I briefly looked around but wasn't able to find something. But that's also no surprise to me, since the use-case is an edge-case that is special to WebTestbase::drupalPostForm(), which, for some reason, didn't want to decide whether it wants to deal with either (1) POST data in PHP array form or (2) a prepared POST data string. Instead of making a clean decision on whichever standard, it chose to ask for a POST data input array that is a undefined mixture and in the middle of both. ;)

    I would totally +1 a follow-up issue for exploring (1) whether we're able to bake that POST data array-flattening logic directly into drupalPostForm(), and while being there, (2) whether we aren't actually able to rely on the (recently added) built-in PHP array to POST data conversion of the cURL extension.

    In any case, the separate helper method is needed for now, so as to allow interactive installer tests to supply $edit values suitable for drupalPostForm().

  13. +1 for investigating whether we can force-hide Simpletest module on production sites. See you over in #1537198: Add a Production/Development Toggle To Core :)
  14. Slightly OT here, but yet:

    + // DrupalKernel relies on global $config_directories and requires those

    something we should tackle in the bootstrap cleanup - moving from globals in settings.php to the kernel and then dropping.

    Any ideas and improvements are certainly a good idea and welcome on that topic, although I'm very skeptical whether that would fundamentally change the gist of the quoted comment. — Whatever the source is, DrupalKernel requires the information to get injected, and that source storage has to exist already.

    Closely related:
    #1757536: Move settings.php to /settings directory, fold sites.php into settings.php
    #2160057-26: Greatly improve the usability of the rebuild.php script

  15. re: Missing phpDoc blocks for properties + methods in InstallerTest & Co:

    As those class properties are essentially custom right now, my plan was to defer those clean-ups to a follow-up issue that turns the revised InstallerTest into a new + formal InstallerTestBase class (provided by Simpletest).

    See #2175459: Convert Drupal\system\Tests\InstallerTest into a new Drupal\simpletest\InstallerTestBase

  16. #2173801: Installing in a different language than English takes forever to complete could additionally resolve that @todo.
  17. ^^
  18. ^^
  19. re: Added test assertion related to #2108623: Installing via the UI no longer logs in user 1 every time:

    The fact that tests are passing with this assertion merely means that the issue does not occur when executing the interactive installer with disabled JS. We need to continue to debug that issue over there.

  20. + // DisplayBag may return NULL if a plugin was not found.

    sounds like a missing module dependency in a test? ie a test view uses a plugin from a module that isn't installed?

    I extensively debugged that one (for almost an entire day), and originally thought the failure would be caused by a malformed views test display configuration (all of the views test config files are in fact outdated), and I verified that the views_test module is actually enabled when ViewsListController iterates over the DisplayBag.

    Now, the actual assertions in the feed display test did actually pass. The test only happens to contain an initial straw drupalGet() to the basic edit page of the test view (without any comments or assertions), and only on that edit page, a fatal error was triggered.

    Further studying the code of DisplayBag and PluginBag, there are various conditions in which the Iterator can return NULL instead of a plugin instance. When manually iterating over a PluginBag, the loop has to take that possibility into account.

    The foreach loop over DisplayBag in ViewListController::getDisplayPaths() did not care for that, so simply checking for whether the Iterator yielded NULL instead of a display appears to be the adequate fix.

    That said, I did not check whether this fatal error also occurs in HEAD and was simply not noticed, because in HEAD, the PHP error.log is set up much later (after the kernel and container was instantiated and after most of the modernized code in HEAD has been executed already).

  21. The missing inclusion of /core/vendor/autoload.php in rebuild.php breaks the rebuild.php script with this patch, because drupal_bootstrap(DRUPAL_BOOTSTRAP_CONFIGURATION) now calls into drupal_valid_test_ua(), which uses the Crypt class. Every front-controller script in D8 has to include /core/vendor/autoload.php first. Therefore, that is a required fix here.
larowlan’s picture

Thanks for the explanations, and the follow-up issues.
There is already an issue to remove private files dir config from the UI, certain I saw it on my rss feed earlier in the week. So that can stay as is.

Re the bootstrap cleanup todo, yeah I'm being sneaky, its not warranted, but was worth a try :)

Not applicable; You commented on removed code ;)

Yes I'm saying I don't think we should remove it, its another measure to prevent tests stuffing your prod db.

In any case, the separate helper method is needed for now, so as to allow interactive installer tests to supply $edit values suitable for drupalPostForm().

Agreed, any effort spent on cleaning up curl handling in simpletest is ultimately a waste, would be better to leave it as and keep moving towards testing with a Request object instead of a HTTP request.

my plan was to defer those clean-ups to a follow-up issue that turns the revised InstallerTest into a new + formal InstallerTestBase class

I think the docs gate dictates they have to go in now, but can wait till the next re-roll obviously.

So other than those doc-blocks, I think this is RTBC.

tim.plunkett’s picture

Only looking at the views stuff:

  1. +++ b/core/modules/views/lib/Drupal/views/Tests/Plugin/DisplayFeedTest.php
    @@ -19,7 +19,7 @@ class DisplayFeedTest extends PluginTestBase {
    -  public static $testViews = array('test_feed_display');
    +  public static $testViews = array('test_display_feed');
    

    Why this change?

  2. +++ b/core/modules/views_ui/lib/Drupal/views_ui/ViewListController.php
    @@ -258,6 +258,10 @@ protected function getDisplayPaths(EntityInterface $view) {
    +      // DisplayBag may return NULL if a plugin was not found.
    +      if (!isset($display)) {
    +        continue;
    +      }
    

    This is true, but worrisome. I guess it's okay here...

I don't know why all the views config changes are needed in this issue.

sun’s picture

FileSize
136.79 KB
13.68 KB

re: #58: @larowlan:

Yes I'm saying I don't think we should remove it [the TestBase::$setupDatabasePrefix property], its another measure to prevent tests stuffing your prod db.

Those fragile class properties and return values are replaced with throwing actual exceptions, halting test execution. Any exception thrown in prepareEnvironment() causes the entire test class to be aborted immediately by TestBase::run().

re: #59: @tim.plunkett:

Thanks for your review!

1. Reverted those changes and created #2175869: Rename views.view.test_feed_display.yml for consistency


  1. Cleaned up unnecessary @todos and commented out code in WebTestBase::setUp().
  2. Added phpDoc to InstallerTest and InstallerTranslationTest.
  3. Reverted unnecessary rename and update of views test_feed_display config file.
  4. Changed visibility of TestBase::prepareDatabasePrefix() and changeDatabasePrefix() to private, since internal to TestBase now.
  5. Fixed stale comment in SimpleTestTest.
sun’s picture

FileSize
136.03 KB
776 bytes

Final adjustment:

  1. Reverted @todo in SystemListingInfo::profiles() → to be hashed out in #2175391: ExtensionDiscovery::getProfileDirectories() explicitly skips parent installation profile in installer, defeating its purpose.

Any other issues, or can we move forward here?

This issue blocks #1757536: Move settings.php to /settings directory, fold sites.php into settings.php

chx’s picture

I have no idea how to review this. While the changes are good, there are so many changes in here that normally would go into separate patches: drupal_settings_initialize losing the globals and their move to a separate _drupal_request_initialize function (which is called only once so I do not readily see the benefit of this), db_prefix vs prefix, the fix in file_save_htaccess, the keep artifacts option. Is the completely rewritten installertests necessary for this change or could that be a followup? Could we please make this more reviewable?

sun’s picture

FileSize
119.71 KB
20.87 KB

OK, I've removed all changes that are not strictly required here:

  1. Reverted clear.artifacts setting. → #2176023: Allow to keep artifacts after a test run (test site + database)
  2. Reverted verbose header output fixes. → #2176043: WebTestBase does not show request headers in verbose output + response header output is borked
  3. Reverted run-tests.sh error handler. → #2176057: Use a proper CLI-compatible error handler for run-tests.sh

Regarding the other mentioned changes:

  1. drupal_settings_initialize losing the globals and their move to a separate _drupal_request_initialize function (which is called only once so I do not readily see the benefit of this)

    While _drupal_request_initialize() is indeed only called once, WebTestBase::setUp() calls into drupal_settings_initialize(), so as to (re)load the settings.php of the test site and properly re-initialize Settings, $databases, $config_directories and $conf, in the exact same way a regular bootstrap would.

    However, the global state manipulations for cookie_domain & Co performed in _drupal_request_initialize() cannot be re-executed and are therefore moved into a separate function.

  2. db_prefix vs prefix

    This mismatch between global $databases and actual database connection info broke WebTestBase after executing the installer and reloading settings.php. I first attempted to dynamically adjust all code for it, but that resulted in plenty of ugly conditional code that was scattered across the system. Hence, I concluded it is much easier and cleaner to fix the inconsistency (and eliminate a whole bunch of @todos related to it).

    With this patch, only a single @todo remains: PIFR (and possibly Drush) are still passing 'db_prefix'.

  3. fix in file_save_htaccess

    The Boolean return value was added to allow simpletest_requirements() to check whether the /sites/simpletest/.htaccess file could be written (to output an error otherwise). So that adjustment happened in response to your earlier security concerns.

    The $htaccess_path adjustment for the case of a stream wrapper URI merely prevents a triple-slash path of e.g. public:///.htaccess, which I first assumed to be the cause of test failures (but wasn't).

I hope this helps.

sun’s picture

FileSize
118.95 KB
1.05 KB
+++ b/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.php
@@ -1887,8 +1944,6 @@ protected function drupalHead()
-    // Ensure that any changes to variables in the other thread are picked up.
-    $this->refreshVariables();

Sorry, something went wrong in those reversions. I did not mean to delete that code.

chx’s picture

Let me help. I filed #2176141: Add a return value to file_save_htaccess() for you. Any other issues we can file ?

Every time I look at this patch I find something. Why did we add the vendor_autoload to core/rebuild.php ?

chx’s picture

chx’s picture

Please disregard everything I said, I already won't fixed the one in #68 and I am unfollowing this issue too.

sun’s picture

While not as complete as here, I managed to additionally extract #2176131: Database configuration form in installer still uses 'db_prefix' instead of 'prefix'

larowlan’s picture

I think this is ready but we should give security team a chance to respond, will ping a few members on Monday

sun’s picture

To perhaps clarify, @chx's temper tantrum does not appear to have been caused by this patch/issue or any particular interaction related to this issue. Not sure what happened.

But @chx is right in that I ran into a good dozen of bugs, hard problems, and inconsistencies that apparently exist throughout core and the testing framework. And in order to make this change possible, I had to attack all of them in one fell swoop.

That certainly makes reviews of this change proposal harder than they need to be. I certainly should have created separate issues for everything else on the go. But you might be familiar with being on a "ride". And given the procedural/organizational overhead of any particular issue, it's very hard to resist the need of "Oh my, let's just simply change those few lines of broken code."

To make up for that, here's a list of separate semi-RTBC and RTBC fixes:

  1. #1376122: Stream wrappers of parent site are leaking into all tests
    → blocks #2176141: Add a return value to file_save_htaccess()
  2. #2176131: Database configuration form in installer still uses 'db_prefix' instead of 'prefix'
  3. #2176151: rebuild.php does not include /core/vendor/autoload.php
  4. #2175869: Rename views.view.test_feed_display.yml for consistency

I additionally tried to extract the TestBase::prepareEnvironment() changes into #2170023: Use exceptions when something goes wrong in test setup, but that did not succeed — testbots blew up in many different ways. I'll continue to try, but I do not have much hope.

So if we can get those in, then I'll be able to remove many little changes here that are a bit "out of scope" for this issue and will make reviews easier.

sun’s picture

Thanks to @tstoeckler who spotted the missing piece, the test environment setup changes are now cleanly extracted, too, and ready for review:

#2170023: Use exceptions when something goes wrong in test setup

tstoeckler’s picture

Oops, forgot the patch.

sun’s picture

Issue summary: View changes
Priority: Normal » Major
Status: Needs review » Postponed
sun’s picture

Status: Postponed » Needs review
FileSize
104.93 KB
15.04 KB

All except of one spin-offs/dependencies have landed - the last remaining: #2176141: Add a return value to file_save_htaccess()

  1. Merged latest 8.x
  2. Updated recently revised BrokenSetUpTest for new test environments.
  3. Removed obsolete $during_test argument from simpletest_log_read() and read the error.log before the test site is deleted.
  4. Fixed LanguageNegotiationInfoTest *again* to properly synchronize altered negotiation info between test runner and child site.
sun’s picture

FileSize
102.66 KB

And the last dependency is gone too now :-)

No interdiff, since merging HEAD just simply eliminated some hunks.

sun’s picture

This patch has been reviewed a couple of times now... I'd love to proceed with #1757536: Move settings.php to /settings directory, fold sites.php into settings.php as soon as possible → RTBC? :)

moshe weitzman’s picture

I reviewed the Issue Summary (quite thorough) and the patch and found nothing to complain about.

However, Cloud hosts like Acquia, Pantheon, etc. do not currently let you write to /sites except in a "live development" mode. You can only writes to the files subdir. I think it is OK to require this mode when running tests, but I'll leave this open for a day before I RTBC it in case anyone objects.

sun’s picture

Cloud hosts like Acquia, Pantheon, etc. do not currently let you write to /sites except in a "live development" mode. You can only writes to the files subdir. I think it is OK to require this mode when running tests

Thanks @moshe, that's interesting.

We could, theoretically, also use a path of '/files/$prefix' (or any other path) — as long as it is hard-wired to Drupal's document root directory; i.e., as long as it does not depend on environment information of the parent site (test runner).

That said, in light of the original/parent issue that triggered all of this, #1757536: Move settings.php to /settings directory, fold sites.php into settings.php...:

In particular, once we have a root /settings.php + root /files directory, we'd be in a position to assume that the root /files directory exists, and thus, the test site directory could as well be /files/simpletest/$prefix.

Funnily, that would technically be the exact same location as in HEAD now — "just" with the difference that it's an actual, re-routed Drupal multi-site directory.

However, this potential change is definitely blocked on #1757536, and after this patch here has landed, the test site directory path can be changed very easily in a follow-up issue at any time.

The currently used /sites/simpletest/$prefix path is in no way required or locked-down and does not have to stay that way; it merely felt most sensible to use the actual multi-site directory for an actual test site directory. If we find a better location later on, then we can easily use that (as long as it is hard-wired to Drupal's document root).

The important part of this change here is that each (web) test actually gets a true and real site directory, so as to remove each and every possibility and requirement of having to share any kind of information between the test runner and the child site under test:

→ If drupal_valid_test_ua() returns TRUE, Drupal is not even aware of the parent site; the entire environment is re-routed to the test site directory, before any other subsystems are bootstrapped, at the earliest possible point in time.

larowlan’s picture

Have reviewed the code (again) and happy with it.
Manual testing now.

larowlan’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
62.98 KB
37.39 KB

Manual testing observations

1) Can no longer enable/install Simpletest module on simplytest.me - see screenshot

how much would would a woodchuck chuck?

2) Testing locally, simpletest UI works fine for a mix of phpunit and web tests.

I would walk 500 miles

tstoeckler’s picture

Oops, and this was a human fail. Wanted to re-upload the patch...

sun’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
102.66 KB

Looks like yet another random testbot OS failure. Anyway, merged 8.x (cleanly).

sun’s picture

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

FileSize
102.62 KB

Merge branch '8.x' into test-site-2171683-sun

sun’s picture

96: test.site_.96.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 96: test.site_.96.patch, failed testing.

sun’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
102.62 KB

Merge branch '8.x' into test-site-2171683-sun

sun’s picture

FileSize
102.62 KB

Merge branch '8.x' into test-site-2171683-sun

catch’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/modules/views_ui/lib/Drupal/views_ui/ViewListController.php
    @@ -251,6 +251,10 @@ protected function getDisplayPaths(EntityInterface $view) {
    +      // DisplayBag may return NULL if a plugin was not found.
    

    This change looks unrelated?

  2. +++ b/core/modules/system/lib/Drupal/system/Tests/System/ErrorHandlerTest.php
    @@ -121,6 +124,9 @@ function testExceptionHandler() {
    +    unlink(DRUPAL_ROOT . '/' . $this->siteDirectory . '/error.log');
    

    Why not drupal_unlink() - same goes for other raw file operations in the patch. If there's a good reason, needs a comment.

bzrudi71’s picture

Most likely all patches to make Drupal install on PostgreSQL are in HEAD now. However even before testbot will show first results we know of problems with failing tests because of to long test names, see #998898: Make sure that the identifiers are not more the 63 characters on PostgreSQL. As this is more or minor a rewrite, can this be taken into account please? At least the IMO ways-to-long "simpletest_some_random_number" prefix should be shorten already as a first step within this patch...

sun’s picture

  1. Created #2189453: Fatal error: Call to a member function hasPath() on a non-object in ViewListController::getDisplayPaths()
  2. All of those test file operations are intentionally not using drupal_ file functions, so as to avoid the built-in @error-suppression. In case the file operation fails, the test did not behave as expected.

    I will try to come up with a short single-line code comment to clarify those usages.

  3. Sorry, but no. :) The existing database table prefixing of Simpletest is not touched at all by this issue here, so any change to that is out of scope for this issue.

    However, I just followed the issue you referenced, and we can happily look into possible solutions. The changes of this patch here might help us to do so.

sun’s picture

Status: Needs work » Needs review
FileSize
101.85 KB
7.38 KB
  1. Removed obsolete change in ViewListController.
  2. Added (consistent) comments to explain raw file operations.
larowlan’s picture

Status: Needs review » Reviewed & tested by the community

back to rtbc if green

catch’s picture

Status: Reviewed & tested by the community » Fixed

Changes work for me. Committed/pushed to 8.x, thanks!

catch’s picture

Status: Fixed » Needs work

This broke the webtest runner with js enabled. Spotted by Alex Pott and confirmed by me:

Exception: Database name field is required. Database username field is required. in install_run_task() (line 640 of /Users/catch/Sites/8.x/core/includes/install.core.inc).

alexpott’s picture

Status: Needs work » Needs review
FileSize
102.83 KB
1001 bytes

The attached patch allows webtests to be run again on PHP when pdo_pgsql is installed. There was some debate about removing the required attribute from the driver fields before on #2101427: Follow-up: Browser validation error with Chrome on hidden required fields, even on forms that don't allow client-side validation - David Rothstein had specific concerns https://drupal.org/node/2101427#comment-8013587. I'm not sure I agree. We still have server validation of database credentials and users with JS will still have the helpful required fields.

sun’s picture

Please excuse my ignorance, but I'm not able to follow recent comments:

  1. What is a "webtest runner with js enabled"?
  2. If by that a Selenium-alike test environment is meant, then how is that related to Simpletest?
  3. How is the stated error message related to this patch, given that (1) the actual database configuration form is not touched at all, (2) all Simpletest web tests for the interactive installer in core are passing, and (3) manual installation works fine and correctly (even with a different db engine)?

In case that "webtest runner with js enabled" thing happens to piggy-back on Simpletest's test environment setup and uses the simpletest* HTTP User-Agent for any reason, then can I only assume that it needs to be updated to read the private key for authentication from the new .htkey file in the test site directory.

Aside from that, I'm not able to make sense of the additional change in #108 — the install_settings_form() uses #limit_validation_errors to limit validation errors to the section of the selected database driver, so removing any #required properties has no effect to begin with.

Overall, I've the impression that the stated error could rather be related to #2176131: Database configuration form in installer still uses 'db_prefix' instead of 'prefix' — and/or a completely different issue.

Long story short: I'm confused. :)

alexpott’s picture

1. Javascript will make the database name and username fields required - with the patch in #108 we remove server side validation. Which allows the non-javascript test runner to work if pdo_pgsql is installed and enabled in PHP.
2. See answer to 1
3. It is totally related. Just enable pdo_pgsql and try and run a webtest.

sun’s picture

FileSize
103.53 KB
2.21 KB

@catch, @alexpott, and @Berdir clarified to me that the reported problem only occurs when you have the PHP pdo_pgsql extension is enabled.

This patch removes a global $database override hack from TestBase::prepareEnvironment(), because WebTestBase::setUp() now just simply invokes the installer in a 100% standard way without any kind of overrides elsewhere in the system. It passes the database form values into the non-interactive installer parameters, but HEY, the installer never ever uses that information. :)

→ Fixed install_settings_form() does not respect non-interactive installer parameters and relies on global $databases instead.

sun’s picture

FileSize
104.5 KB
3.11 KB

install_settings_form() still needs to account for global $databases, in case database connection settings have been prepared in settings.php already.

The last submitted patch, 111: test.site_.110.patch, failed testing.

alexpott’s picture

FileSize
655 bytes
102.31 KB

Here is an alternate approach that doesn't make any changes to install_settings_form and just fixes the non-interactive installer.

Interdiff is to the patch that was committed.

Status: Needs review » Needs work

The last submitted patch, 114: 2171683.114.patch, failed testing.

alexpott’s picture

Status: Needs work » Reviewed & tested by the community

@sun is correct #114 is a dud - will break form altering install profiles and will fail tests.

Reviewed and manually tested #112 and spent far too much thinking about the code - it looks great.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed #112 to 8.x, thanks!

xjm’s picture

Title: Remove all Simpletest overrides and rely on native multi-site functionality instead » Change record: Remove all Simpletest overrides and rely on native multi-site functionality instead
Status: Fixed » Active
Issue tags: +Needs change record, +Missing change record

Surely this needs some change record action? :)

Les Lim’s picture

Status: Active » Needs review

Wrote up a draft change record explaining the need for a "sites/simpletest" directory if Drupal is unable to create the directory itself. Otherwise it doesn't seem like there should be any impact on test developers.

https://drupal.org/node/2193261

tstoeckler’s picture

Status: Needs review » Fixed

Thanks @Les Lim. I added a short paragraph about the .htaccess protection and published the change notice.

Les Lim’s picture

Title: Change record: Remove all Simpletest overrides and rely on native multi-site functionality instead » Remove all Simpletest overrides and rely on native multi-site functionality instead
Issue tags: -Needs change record, -Missing change record

de-tagifying.

Berdir’s picture

I have troubles running tests on one of my systems now, see #2194071: WebTestBase::setUp() fails with PHP 5.5 and enabled opcache. Would appreciate feedback by others who are running 5.5.

amateescu’s picture

Should we be worried that this patch added ~1h to our testing times? Before it, the full testsuite ran in about 1h on fast bots and 1.5h on slow ones; after it, I'm seeing only 2h - 2.5h. And #2188661: Extension System, Part II: ExtensionDiscovery doesn't seem to make up for it..

Berdir’s picture

Strange, looking into it, but I can't see anything obvious.

It is a bit slower with this but not much (I'm seeing 20s vs 21s for a test with xhprof enabled, on laptop that isn't plugged in, so not too reliable), the main difference seems to be that we're using the database cache during the installer now, which adds ~1800 deleteTags() queries to the Database backend, taking 1.3s for me. That doesn't explain the numbers you're seeing (Which I believe, we also have lots of random fails since this happened).

amateescu’s picture

You're right, the difference is not as big as I thought at first, it's only 5-10min for fast bots and 20-30min for slow bots.

longwave’s picture

I think this patch has broken all contrib module test runs on testbot. Testbot downloads the contrib module to sites/default, so it's not available to the sites/simpletest multisite instance. See e.g. https://qa.drupal.org/pifr/test/725203 which has 0 passes because none of the modules can be enabled.

I guess this is a testbot issue, though?

edit: filed #2194271: D8: Make testbot check out projects into top-level /modules directory

andypost’s picture

I see the slowness in testing and talked about it in IRC, so maybe this one caused so filed #2194267: Fix testing speed regressions

Berdir’s picture

sun’s picture

Testbot downloads the contrib module to sites/default, so it's not available to the sites/simpletest multisite

*facepalm* ;) — For everyone who missed @longwave's comment edit, he filed a testbot issue:

#2194271: D8: Make testbot check out projects into top-level /modules directory

Status: Fixed » Closed (fixed)

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