Suggested commit message: Patch #1949724 by webchick, chx: Test more of update.php

At least we need to test update_free_access TRUE and update_free_access FALSE. We need to check, in both cases, for anonymous and non-anonymous users. We need to check for having administer software updates and not having administer software updates. As a plus we need to check for industrious users pre-creating and pre-adding their config directories to settings.php as I think there's a separate code path for that. We need to populate various caches , theme registry, bootstrap system list, module implements etc.

The strangest code paths exist. I now know why automated tests do not fail: they insert a session and do not set update_free_access to TRUE. If you do not set update_free_access to TRUE then update_access_allowed adds the path to the user module to the kernel and from there to the classloader so that it can check for user_access. That path allows for the User entity class to load which is a pre-requisite for successful bootstrap of anonymous users because drupal_anonymous_user returns a User entity.

The credit for the override mechanism is webchick's. She came up with it on IRC. Standing on the shoulders of giants.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

chx’s picture

Issue summary: View changes

Updated issue summary.

chx’s picture

Status: Active » Needs review
FileSize
2.78 KB

This is the first step on a very long journey: it will allow in the future the testing for non-precreated config directories. Up until then I can sell this is as a performance enhancement ;) although one that is so small to be not perceivable.

chx’s picture

The passing test contains #1948650-6: Update is broken. The failing test doesn't. The reason it's not in that issue is, again, is that I want that issue to move faster and this might cause some controversy -- and also because it will need more, more, more tests.

Status: Needs review » Needs work

The last submitted patch, 1949724_2-pass.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
7.25 KB
6.23 KB

Opsie, wrong roll.

xjm’s picture

Priority: Normal » Critical

This is absolutely critical. Apparently this is necessary to add test coverage for #1948650: Update is broken.

xjm’s picture

Issue summary: View changes

Updated issue summary.

chx’s picture

Issue summary: View changes

Updated issue summary.

chx’s picture

webchick’s picture

LOL @ the issue summary. Methinks you're giving me entirely too much credit for asking some stupid questions. :D

But AWESOME to see this being worked on!!

chx’s picture

FileSize
10.32 KB

Here's one that generates its own config_directories.

chx’s picture

FileSize
10.69 KB

Much less debug, much more doxygen.

dawehner’s picture

+++ b/core/includes/bootstrap.incundefined
@@ -2617,9 +2614,50 @@ function drupal_valid_test_ua($new_prefix = NULL) {
+ * Very strictly for internal use only.

Is it common to not start with the description this method is actually doing? It seems to be better to have first a one liner what's actually going on.

+++ b/core/includes/bootstrap.incundefined
@@ -2617,9 +2614,50 @@ function drupal_valid_test_ua($new_prefix = NULL) {
+function _drupal_test_overrides($test_prefix, $config) {

What about name it _drupal_load_test_overrides? test could also be a verb in this context.

+++ b/core/includes/bootstrap.incundefined
@@ -2617,9 +2614,50 @@ function drupal_valid_test_ua($new_prefix = NULL) {
+    $config_directories = array();

Do we really want to override the existing config_directories in the else case?

+++ b/core/includes/bootstrap.incundefined
@@ -2617,9 +2614,50 @@ function drupal_valid_test_ua($new_prefix = NULL) {
+    include $filename;
...
+    include $filename;

I'm wondering whether this should be include_once here.

+++ b/core/includes/update.incundefined
@@ -107,7 +107,6 @@ function update_prepare_d8_bootstrap() {
-

Let's skip this here.

+++ b/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.phpundefined
@@ -157,6 +157,15 @@
+   * Whether the config directories are pregenerated.
...
+  protected $pregeneratedConfigDirectories = 'Y';

Just wondering why we can't use a boolean here, or something else? Y|N feels really confusing.

+++ b/core/modules/system/lib/Drupal/system/Tests/Upgrade/BareMinimalAnonymousUpgradePathTest.phpundefined
@@ -0,0 +1,33 @@
+ * Definition of Drupal\system\Tests\Upgrade\BareMinimalAnonymousUpgradePathTest.

Contains ...

+++ b/core/modules/system/lib/Drupal/system/Tests/Upgrade/BareMinimalAnonymousUpgradePathTest.phpundefined
@@ -0,0 +1,33 @@
+

empty line

+++ b/core/modules/system/lib/Drupal/system/Tests/Upgrade/BareMinimalAnonymousUpgradePathTest.phpundefined
@@ -0,0 +1,33 @@
+class BareMinimalAnonymousUpgradePathTest extends BareMinimalUpgradePathTest {

Some kind of description for the class would be nice.

+++ b/core/modules/system/lib/Drupal/system/Tests/Upgrade/BareMinimalAnonymousUpgradePathTest.phpundefined
@@ -0,0 +1,33 @@
+      'name'  => 'Basic minimal profile upgrade, free access',
+      'description'  => 'Basic upgrade path tests for a minimal profile install with a bare database and update_free_access set to TRUE.',

sorry, there are some unneeded spaces here :)

+++ b/core/modules/system/lib/Drupal/system/Tests/Upgrade/BareMinimalNoConfigUpgradePathTest.phpundefined
@@ -0,0 +1,49 @@
+    // Refresh the variables only if the site was already upgraded.
...
+    parent::refreshVariables();

But we refresh them anyway?

+++ b/core/modules/system/lib/Drupal/system/Tests/Upgrade/BareMinimalUpgradePathTest.phpundefined
@@ -95,4 +90,15 @@ public function testBasicMinimalUpgrade() {
+    // Logout and verify that we can login back in with our initial password.
+    $this->drupalLogout();
+  }

Oh I don't see the code which tries to login again.

chx’s picture

FileSize
11.22 KB

Also, as discussed on IRC, override.php dies an early death and now we have settings.php solely. There is no interdiff: it's the same size as the patch itself, so there'd be very little point.

> Is it common to not start with the description this method is actually doing? It seems to be better to have first a one liner what's actually going on.

All hell breaks lose if you call this method with the wrong arguments (and have a well prepared settings.php). But, I agree, it's not unique in this regard so I fixed the doxygen.

> What about name it _drupal_load_test_overrides?

Done.

> Do we really want to override the existing config_directories in the else case?

Yes. update.php runs a !empty check over it.

> I'm wondering whether this should be include_once here.

The extra time is not worth it. And who knows, maybe someone does want to call it within the same request -- likely a test of simpletest itself or somesuch.

> Just wondering why we can't use a boolean here, or something else? Y|N feels really confusing.

Done.

> empty line

I thought we have an empty line after the last method?

> Some kind of description for the class would be nice.

Agreed. Have some to spare :) ? Added them. Test method doxygen too.

> sorry, there are some unneeded spaces here :)

Removed them.

> But we refresh them anyway?

Not anymore.

> Oh I don't see the code which tries to login again.

Renamed and moved comments to clarify.

jibran’s picture

FileSize
10.6 KB

interdiff from 10-12.

chx’s picture

Title: Test more of update.php » Add top overrides and test anonymous / configless updates with it
Berdir’s picture

Love this.

+++ b/core/includes/bootstrap.incundefined
@@ -2617,9 +2619,50 @@ function drupal_valid_test_ua($new_prefix = NULL) {
+  $filename = conf_path() . '/files/' . $path_prefix . '/settings.php';
+  if (file_exists($filename)) {

This idea isn't completely new.

We had it before but did not extend an existing file but copy it completely. That idea was then correctly turned down (by you I think) because the result was that we've put the database connection info into a file in files/.

And, this will happen again with this when we convert $databases to $settings and test the conversion of that.

What I was thinking about for a while is if we could put the settings.php into the php/ directory...

+++ b/core/modules/system/lib/Drupal/system/Tests/Upgrade/BareMinimalAnonymousUpgradePathTest.phpundefined
@@ -0,0 +1,42 @@
+ * Definition of Drupal\system\Tests\Upgrade\BareMinimalAnonymousUpgradePathTest.

Usual nitpick, should be Contains \Drupal\... ;)

chx’s picture

> And, this will happen again with this when we convert $databases to $settings and test the conversion of that.

drupal_rewrite_settings is already ready for that, almost all the code that is necessary is in http://drupal.org/files/1951068_11.patch already -- needs another if (drupal_valid_test_ua()) unset($database password) and lo! we will be dumping all the settings without password into the override settings.php while the password will come from the real one.

I thought of using the php storage too but deemed unnecessary complex.

alexpott’s picture

chx’s picture

FileSize
631 bytes
11.22 KB

Fixed nitpick. This should be ready.

alexpott’s picture

I think the approach taken by this patch is awesome - it's what #1576322: page_cache_without_database doesn't return cached pages without the database should have got round to implementing :)

Some minor nits...

+++ b/core/includes/bootstrap.incundefined
@@ -429,6 +429,11 @@ function conf_path($require_settings = TRUE, $reset = FALSE) {
+  // Keep the conf_path alive across drupal_static_reset() calls.
+  if (class_exists('Drupal\\Component\\Utility\\Settings') && ($simpletest_conf_path = settings()->get('simpletest_conf_path'))) {

Should be 'Drupal\Component\Utility\Settings'

+++ b/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.phpundefined
@@ -157,6 +157,15 @@
+   * @var string
+   *   Set to N if you want the child Drupal to not use the pregenerated
+   *   config directory path.

This should be set to FALSE and @var bool

+++ b/core/includes/bootstrap.incundefined
@@ -2617,9 +2619,50 @@ function drupal_valid_test_ua($new_prefix = NULL) {
+ * Overrides some very basic settings.

I think we need to qualify what "some" is... perhaps: Overrides low level and environment specific configuration.

+++ b/core/modules/system/lib/Drupal/system/Tests/Upgrade/BareMinimalNoConfigUpgradePathTest.phpundefined
@@ -0,0 +1,52 @@
+ * Tests the database upgrade path without creating config directories.

This is a bit bikesheddy but more precise... Tests the upgrade path without prior creation of config directions.

+++ b/core/modules/system/lib/Drupal/system/Tests/Upgrade/BareMinimalUpgradePathTest.phpundefined
@@ -95,4 +90,14 @@ public function testBasicMinimalUpgrade() {
+   * Assert that the session was kept during update. Also, log out.

Should be Asserts

chx’s picture

FileSize
2.66 KB
11.24 KB
alexpott’s picture

Status: Needs review » Reviewed & tested by the community

This is ready to fly.

effulgentsia’s picture

Wow. The interaction here between conf_path() and _drupal_load_test_overrides() is quite a mind bender, but it does accomplish what it needs to.

+++ b/core/includes/bootstrap.inc
@@ -429,6 +429,11 @@ function conf_path($require_settings = TRUE, $reset = FALSE) {
+  if (class_exists('Drupal\Component\Utility\Settings') && ($simpletest_conf_path = settings()->get('simpletest_conf_path'))) {

Not a commit blocker, but can we add a drupal_valid_test_ua() check in here? Otherwise, if someone were to set $settings['simpletest_conf_path'] in their regular settings.php file, Drupal would behave somewhat unpredictably, since conf_path() would return one thing for a while, then after something randomly invokes it with a reset, would start returning something else.

Also, just noting that this patch loads the simpletest-specific settings.php in addition to, rather than instead of, the parent site's settings.php. Whereas #1576322-29: page_cache_without_database doesn't return cached pages without the database was working towards making it a replacement, in order to get even more isolation from random settings in the parent site. However, if we still want that isolation, that's a separate concern (with its own challenges, like securing the database password), and we shouldn't delay this issue for it.

chx’s picture

FileSize
591 bytes
11.27 KB

Really funny that since #12 I can upload the same patch slightly edited without needing to roll a new one -- that's how small the necessary changes are.

As for overriding, it's an absolute must and a big win over needing to write a new which we tried, several times, and couldn't make it happen.

webchick’s picture

Assigned: chx » catch

Hm. This smells like a patch it'd be good to have catch look at.

chx’s picture

Note that #1872522: Compiled data in PHP storage is cleared too late in drupal_flush_all_caches() is either challenging or impossible to test without this. Neither dawhener nor me could answer the challenge so I will go with impossible.

catch’s picture

+++ b/core/includes/bootstrap.incundefined
@@ -429,6 +429,11 @@ function conf_path($require_settings = TRUE, $reset = FALSE) {
+  // Keep the conf_path alive across drupal_static_reset() calls.
+  if (class_exists('Drupal\Component\Utility\Settings') && ($simpletest_conf_path = settings()->get('simpletest_conf_path')) && drupal_valid_test_ua()) {
+    $conf = $simpletest_conf_path;
+    return $conf;

Can we put the drupal_valid_test_ua() check first? That this is for simpletest only seems the most important of all these, it's also not reflected in the comments.

ParisLiakos’s picture

FileSize
820 bytes
11.28 KB

fixed comments and conditional

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 1949724_27.patch, failed testing.

chx’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
910 bytes
11.49 KB

You can't. It's not going to work as the number of failures above show. It's too early to call. Yes, it's very tricky, so I added comments. Note: interdiff is to #23.

Back to RTBC as there is no change.

xjm’s picture

Title: Add top overrides and test anonymous / configless updates with it » Allow a secondary settings.php file to override the
Assigned: catch » xjm
Status: Reviewed & tested by the community » Needs review
+++ b/core/includes/bootstrap.incundefined
@@ -429,6 +429,14 @@ function conf_path($require_settings = TRUE, $reset = FALSE) {
+  // For testing, keep the conf_path alive across drupal_static_reset() calls.
+  // Also, by checking for the existence of the Settings class, we make sure
+  // that this fires only after the first bootstrap phase is complete,
+  // otherwise drupal_valid_test_ua() does not work.
+  if (class_exists('Drupal\Component\Utility\Settings', FALSE) && ($simpletest_conf_path = settings()->get('simpletest_conf_path')) && drupal_valid_test_ua()) {
+    $conf = $simpletest_conf_path;
+    return $conf;
+  }

This is still really unclear, and it took @effulgentsia explaining it verbally and me reading it about 10 times after that for me to understand it. I'm going to refactor/redocument this hunk.

effulgentsia’s picture

Assigned: xjm » effulgentsia
Status: Needs review » Needs work

Once xjm posts the tweaks she's working on, I'll post some additional tweaks as well.

effulgentsia’s picture

Title: Allow a secondary settings.php file to override the » Allow simpletest child sites to additionally load a test-specific settings.php to allow testing anonymous and configless updates
xjm’s picture

xjm’s picture

Assigned: effulgentsia » xjm

@effulgentsia and I crossposted. :) He'll work on it once I'm done making the code more grokkable. Patch might be first thing tomorrow.

chx’s picture

FileSize
179.03 KB

Let me leave this here.

xjm’s picture

Assigned: xjm » effulgentsia
Status: Needs work » Needs review
FileSize
2.55 KB
12.72 KB

:)

Here's the first cleanup. Not winning any diffstat awards because I replaced a one-line condition with like a 30-line mostly-docs function, but I think this will save future generations the trauma I went through yesterday afternoon.

There's a bunch of other minor documentation cleanups that are needed, but I'll address them after Alex tries his fix.

xjm’s picture

+++ b/core/includes/bootstrap.incundefined
@@ -405,6 +404,42 @@ function conf_path($require_settings = TRUE, $reset = FALSE) {
+  // Otherwise, when the $simpletest_conf_path is set in a valid test request,
+  // return that path.

It would probably make more sense to remove the word "Otherwise" here since the previous comment begins with "ensure," but I'll fix that after @effulgentsia is done.

Status: Needs review » Needs work

The last submitted patch, bootstrap-1949724-38.patch, failed testing.

effulgentsia’s picture

Assigned: effulgentsia » Unassigned
Status: Needs work » Needs review
FileSize
8.77 KB
11.11 KB

This removes the special passing of 'N' in the user agent for emptying $config_directories. Instead, the secondary settings.php can set $config_directories to an empty array if it wants to.

Some other minor cleanup too.

Status: Needs review » Needs work
Issue tags: -D8 upgrade path

The last submitted patch, bootstrap-1949724-41.patch, failed testing.

webchick’s picture

Status: Needs work » Needs review

#41: bootstrap-1949724-41.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +D8 upgrade path

The last submitted patch, bootstrap-1949724-41.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
1.25 KB
11.1 KB

Status: Needs review » Needs work

The last submitted patch, bootstrap-1949724-45.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
4.64 KB
12.74 KB
chx’s picture

Just FYI, the installer patch is up-to-date with the changes in #45 (#47 does not affect me, but I will upgrade to use the new helper) so if this gets in, the installer patch will follow immediately.

webchick’s picture

Overall, this is much more understandable. Thanks!

+++ b/core/modules/system/lib/Drupal/system/Tests/Upgrade/BareMinimalAnonymousUpgradePathTest.phpundefined
@@ -0,0 +1,48 @@
+<?php
+
+/**
+ * Contains \Drupal\system\Tests\Upgrade\BareMinimalAnonymousUpgradePathTest.

+++ b/core/modules/system/lib/Drupal/system/Tests/Upgrade/BareMinimalNoConfigUpgradePathTest.phpundefined
@@ -0,0 +1,65 @@
+<?php
+/**
+ * Contains \Drupal\system\Tests\Upgrade\BareMinimalNoConfigUpgradePathTest.

Only if there's another re-roll... those need a @file. Can also fix on commit.

+++ b/core/modules/system/lib/Drupal/system/Tests/Upgrade/BareMinimalUpgradePathTest.phpundefined
@@ -95,4 +90,14 @@ public function testBasicMinimalUpgrade() {
+  /**
+   * Asserts that the session was kept during update. Also, log out.
+   */
+  protected function assertSessionKept() {
+    $this->drupalGet('user');
+    $this->clickLink(t('Edit'));
+    $this->assertEqual($this->getUrl(), url('user/1/edit', array('absolute' => TRUE)), 'We are still logged in as admin at the end of the upgrade.');
+    $this->drupalLogout();
+  }

Do we need to do all of that to figure out if the session is kept? Can we not just, for example, assertText($user->name) on any page?

Also, why are we logging out at the end of an assertion..? Seems that log out should be back in the original flow? (It's actually unclear to me why we need this helper function at all given that the 4 lines of code therein are only used in one place, and it necessitates 3 lines of code that do nothing in BareMinimalAnonymousUpgradePathTest).

effulgentsia’s picture

It's actually unclear to me why we need this helper function at all

BareMinimalAnonymousUpgradePathTest extends BareMinimalUpgradePathTest. So do lots of other tests. We need BareMinimalAnonymousUpgradePathTest to not do those lines of code, so we move them into a helper function that it can override.

Also, why are we logging out at the end of an assertion..?

That's the flow that's in HEAD: do the upgrade, log out, then log in again. This patch isn't changing that. It's just moving the code into a helper function that can be overridden when testing an upgrade that's initiated by anonymous.

effulgentsia’s picture

Assigned: Unassigned » xjm

Assigning to @xjm since she indicated she wanted another docs pass in #38.

I think this is really close to RTBC. Since this already was RTBC in #21, I would suggest that @chx or anyone else who's ok with the interdiffs since then is eligible to re-RTBC. The bulk of those interdiffs have been mine though, so I'll refrain from doing that.

chx’s picture

Assigned: xjm » Unassigned

assertSessionKept might not be an assert. It checks whether we are still logged in and then logs out, I am not sure what to call it. This is the surefire way of checking for loggedinness, merely checking a username is not enough consider a node posted by the user etc. The logout belongs inside because after this function we need to be logged out but in the new tests we are already logged out before so you can't keep logout in the parent.

chx’s picture

Assigned: Unassigned » xjm

I am fine with the changes so when xjm is done with the changes I will re-rtbc

xjm’s picture

For #49, I think we could probably call the method finishUpgradeSession() or something?

I'll post another patch shortly.

xjm’s picture

Just a few notes for myself:

  1. +++ b/core/includes/bootstrap.incundefined
    @@ -397,6 +404,49 @@ function conf_path($require_settings = TRUE, $reset = FALSE) {
    + * Simpletest may provide a secondary, test-specific settings.php file to load
    + * after (and therefore override variables set by) the primary one used by the
    + * parent site. If the child settings.php does not override $conf_path, then
    + * this function returns FALSE and conf_path() returns the directory of the
    + * primary settings.php. If the child settings.php does override $conf_path,
    + * then _drupal_load_test_overrides() sets the 'simpletest_conf_path' setting,
    + * and this function returns that to conf_path(), causing installations and
    + * upgrades to act on that one.
    

    Thanks @effulgentsia for adding more detail. The first sentence is a little unwieldy so I'll try to reword this a little and maybe split it into two paragraphs.

  2. +++ b/core/includes/bootstrap.incundefined
    @@ -397,6 +404,49 @@ function conf_path($require_settings = TRUE, $reset = FALSE) {
    +  // Ensure that the settings object is available. conf_path() is called once
    +  // before the Settings class is included, and at that point it should still
    +  // load the primary $conf_path.
    +  if (!class_exists('Drupal\Component\Utility\Settings', FALSE)) {
    +    return FALSE;
    +  }
    

    I still feel like this part is a bit fragile. The first time we call conf_path() in drupal_settings_initialize(), we want it to load the real, actual settings.php. Relying on exactly when this class gets included is not at all robust. Now, we still do need this check because settings() will fatal if it's not included, but I'm also thinking we should explicitly check somewhere to make sure it's the first request. Ways to do this might include setting a static (plain static, not drupal_static()), or checking to see if at least one settings.php has been loaded, or something else. I think this can probably be addressed in a followup, so that we can get this in to fix the upgrade path and unblock the installer tests.

    An @see to drupal_settings_initialize() would also not go amiss.

  3. +++ b/core/includes/bootstrap.incundefined
    @@ -397,6 +404,49 @@ function conf_path($require_settings = TRUE, $reset = FALSE) {
    +  // Ensure that this is actually a simpletest request. We can't check this
    +  // before the settings object is available.
    

    Taking with @chx yesterday I realized "the settings object" could be interpreted to mean the class and not the instantiated object, so I'll reword this to "before settings.php is loaded."

  4. +++ b/core/includes/bootstrap.incundefined
    @@ -397,6 +404,49 @@ function conf_path($require_settings = TRUE, $reset = FALSE) {
    +  // Otherwise, when the $simpletest_conf_path is set in a valid test request,
    +  // return that path.
    

    I'll remove the word "Otherwise."

  5. +++ b/core/includes/bootstrap.incundefined
    @@ -2575,6 +2622,43 @@ function drupal_valid_test_ua($new_prefix = NULL) {
    + * Overrides low level and environment specific configuration.
    

    low-level, environment-specific

  6. +++ b/core/includes/bootstrap.incundefined
    @@ -2575,6 +2622,43 @@ function drupal_valid_test_ua($new_prefix = NULL) {
    + * Loads settings.php from the simpletest public files
    + * directory. These files can change the global $conf, the global
    + * $config_directories, the return value of conf_path() and
    + * settings().
    

    Wrapping too early. Also, missing the Oxford comma.

  7. +++ b/core/includes/bootstrap.incundefined
    @@ -2575,6 +2622,43 @@ function drupal_valid_test_ua($new_prefix = NULL) {
    + * @param $test_prefix
    + *   The simpletest prefix.
    

    This parameter needs a datatype declaration.

  8. +++ b/core/includes/bootstrap.incundefined
    @@ -2575,6 +2622,43 @@ function drupal_valid_test_ua($new_prefix = NULL) {
    +  $filename = conf_path() . '/files/' . $path_prefix . '/settings.php';
    

    Comment above here would not go amiss.

  9. +++ b/core/includes/bootstrap.incundefined
    @@ -2575,6 +2622,43 @@ function drupal_valid_test_ua($new_prefix = NULL) {
    +    // This can override $conf, $conf_path, $settings and $config_directories.
    

    Missing Oxford comma again. :)

  10. +++ b/core/modules/system/lib/Drupal/system/Tests/Upgrade/BareMinimalAnonymousUpgradePathTest.phpundefined
    @@ -0,0 +1,48 @@
    +<?php
    +
    +/**
    + * Contains \Drupal\system\Tests\Upgrade\BareMinimalAnonymousUpgradePathTest.
    + */
    

    Add @file.

  11. +++ b/core/modules/system/lib/Drupal/system/Tests/Upgrade/BareMinimalAnonymousUpgradePathTest.phpundefined
    @@ -0,0 +1,48 @@
    +class BareMinimalAnonymousUpgradePathTest extends BareMinimalUpgradePathTest {
    +  public static function getInfo() {
    

    Blank line needed between the class and its first member.

  12. +++ b/core/modules/system/lib/Drupal/system/Tests/Upgrade/BareMinimalAnonymousUpgradePathTest.phpundefined
    @@ -0,0 +1,48 @@
    +    $settings['settings']['update_free_access'] = (object) array(
    +      'value' => TRUE,
    +      'required' => TRUE,
    +    );
    +    $this->writeSettings($settings);
    

    Comment: "Override $update_free_access in settings.php to allow the anonymous user to run the upgrade."

  13. +++ b/core/modules/system/lib/Drupal/system/Tests/Upgrade/BareMinimalAnonymousUpgradePathTest.phpundefined
    @@ -0,0 +1,48 @@
    +    // We are not logged in, nothing to do.
    ...
    +    // We are not logged in, nothing to do.
    

    Comma splice. :) The comment could also be a little clearer. I'll fix.

  14. +++ b/core/modules/system/lib/Drupal/system/Tests/Upgrade/BareMinimalAnonymousUpgradePathTest.phpundefined
    @@ -0,0 +1,48 @@
    +  protected function assertSessionKept() {
    
    +++ b/core/modules/system/lib/Drupal/system/Tests/Upgrade/BareMinimalUpgradePathTest.phpundefined
    @@ -43,12 +43,7 @@ public function testBasicMinimalUpgrade() {
    +    $this->assertSessionKept();
    
    @@ -95,4 +90,14 @@ public function testBasicMinimalUpgrade() {
    +  protected function assertSessionKept() {
    

    And, I'll rename this.

  15. +++ b/core/modules/system/lib/Drupal/system/Tests/Upgrade/BareMinimalNoConfigUpgradePathTest.phpundefined
    @@ -0,0 +1,65 @@
    +<?php
    +/**
    + * Contains \Drupal\system\Tests\Upgrade\BareMinimalNoConfigUpgradePathTest.
    + */
    

    Another missing @file.

  16. +++ b/core/modules/system/lib/Drupal/system/Tests/Upgrade/BareMinimalNoConfigUpgradePathTest.phpundefined
    @@ -0,0 +1,65 @@
    +    $settings['conf_path'] = (object) array(
    +      'value' => $this->public_files_directory,
    +      'required' => TRUE,
    +    );
    +    $settings['config_directories'] = (object) array(
    +      'value' => array(),
    +      'required' => TRUE,
    +    );
    +    $this->writeSettings($settings);
    

    Similar comment would be good.

xjm’s picture

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

13.37 KB

This patch has to be ready ;)

Looked through it again, there is a lot of really great documentation for this, which is important, also like the updated method name for the upgrade path tests.

This is by the way a pattern that we already added in the mail upgrade path tests with checkCompletionPage() (just the other way round, empty default implementation) to allow upgrade path implementations to do additional checks on that page before it continues. So this seems consistent to me.

chx’s picture

Thanks everyone for working on this! Awesome work.

Damien Tournoud’s picture

I might as well be missing something, but why not simply pass the settings.php file as a request parameter (aka X-Drupal-Config-Path: ...), instead of doing this (arguably very brittle) dance? I really *hate* making the bootstrap process even more fragile, especially when there is still so much work required to integrate the Symfony bootstrap process properly.

xjm’s picture

I might as well be missing something, but why not simply pass the settings.php file as a request parameter (aka X-Drupal-Config-Path: ...), instead of doing this (arguably very brittle) dance? I really *hate* making the bootstrap process even more fragile, especially when there is still so much work required to integrate the Symfony bootstrap process properly.

Yeah, I'd agree about it being brittle. How much would we need to do to change it as you suggest?

chx’s picture

I do not readily see the win -- the complexity arises from the fact of the override, the conf_path override is a relatively small detail. But. A bigger problem. How do you plan reading the request headers? getallheaders only works with Apache and you need 5.4.0 to support FastCGI.

xjm’s picture

Assigned: xjm » Unassigned

Maybe let's get this in as-is to unblock the installer testing, and then discuss #59 and my point 2 from #55 in a followup?

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Yeah, I agree. The win here is massive. But a follow-up to find a more optimal way of doing these cartwheels works for me, as long as #61 doesn't shut the idea down entirely.

Committed and pushed to 8.x. YEAH!!! Great job, chx!!!! Bring on the installer/updater tests. :D

xjm’s picture

Issue tags: +Needs followup
Damien Tournoud’s picture

Status: Fixed » Reviewed & tested by the community
Issue tags: -Needs followup

I do not readily see the win -- the complexity arises from the fact of the override, the conf_path override is a relatively small detail.

The win is to not introduce a backward dependency between settings and config, which would make an already messy bootstrap process even messier.

But. A bigger problem. How do you plan reading the request headers? getallheaders only works with Apache and you need 5.4.0 to support FastCGI.

@chx: I don't see the problem. All HTTP request headers are present in $_SERVER. That's guaranteed on all the SAPIs, because it comes directly from CGI/1.1:

   Meta-variables with names beginning with "HTTP_" contain values read
   from the client request header fields, if the protocol used is HTTP.
   The HTTP header field name is converted to upper case, has all
   occurrences of "-" replaced with "_" and has "HTTP_" prepended to
   give the meta-variable name.
effulgentsia’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +Needs followup

I suspect #65 didn't intend to change the issue attributes, so restoring them.

Without seeing a patch for #59, I also don't see how a header would simplify. The complexity comes from having 2 settings.php files, and wanting conf_path() to return the path to the first one until after the second one is loaded, and then maybe return the path to the second one, maybe not. I'm not readily seeing how a header would decouple that mess, but I'd be happy to look at a patch if one gets posted.

Damien Tournoud’s picture

The complexity comes from having 2 settings.php files, and wanting conf_path() to return the path to the first one until after the second one is loaded, and then maybe return the path to the second one, maybe not. I'm not readily seeing how a header would decouple that mess, but I'd be happy to look at a patch if one gets posted.

That's exactly what I'm saying. Why do we need this mess in the first place? Just have a request header that specify which settings.php file to load, and be done with it. This "let's load two configuration files" dance is both dangerous (because it's going to get in the way of the badly needed refactoring of the bootstrap) and unnecessary.

But again, I might be missing something.

Berdir’s picture

A single file was the approach tried in #1576322: page_cache_without_database doesn't return cached pages without the database, one problem there was that we then wrote the database password into a file within the public files directory. See comment #32 there.

chx’s picture

Yes, the one file approach is definitely a no-go; check the installer test patch -- it has test specific code explicitly removing the password from the simpletest-specific settings.php

Damien Tournoud’s picture

Ok. Then how does passing the settings overrides directly inside the request header (in serialized form) sound?

chx’s picture

Both the the installer and the update tests need to write *some* settings.php as that's part of what they do and what we test (the installer test, check it, has a lot of comments on how the test-specific passwordless settings rewrite code is small and contained -- we do not want to have a simpletest-specific, tested code path and a generic nontested one). Both of them set the config dir for their prefixed D8 install in this settings.php. So, we need to load that settings.php anyways. Unless you want to load it in the parent and re-create the header based on it, but really, what do you win?

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary.