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.
Comment | File | Size | Author |
---|---|---|---|
#56 | bootstrap-1949724-56.patch | 13.37 KB | xjm |
#56 | interdiff-bootstrap-56.txt | 7.81 KB | xjm |
#47 | bootstrap-1949724-47.patch | 12.74 KB | effulgentsia |
#47 | interdiff.txt | 4.64 KB | effulgentsia |
#45 | bootstrap-1949724-45.patch | 11.1 KB | effulgentsia |
Comments
Comment #0.0
chx CreditAttribution: chx commentedUpdated issue summary.
Comment #1
chx CreditAttribution: chx commentedThis 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.
Comment #2
chx CreditAttribution: chx commentedThe 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.
Comment #4
chx CreditAttribution: chx commentedOpsie, wrong roll.
Comment #5
YesCT CreditAttribution: YesCT commentedrelated #1948390: [Meta] Upgrade to D8 from D7: regular testing needed on various OSs with different versions of php and other unique situations
Comment #6
xjmThis is absolutely critical. Apparently this is necessary to add test coverage for #1948650: Update is broken.
Comment #6.0
xjmUpdated issue summary.
Comment #6.1
chx CreditAttribution: chx commentedUpdated issue summary.
Comment #7
chx CreditAttribution: chx commentedPostponed #1941000: update_module_enable() does not update ModuleHandler::$moduleList on this.
Comment #8
webchickLOL @ 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!!
Comment #9
chx CreditAttribution: chx commentedHere's one that generates its own config_directories.
Comment #10
chx CreditAttribution: chx commentedMuch less debug, much more doxygen.
Comment #11
dawehnerIs 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.
What about name it _drupal_load_test_overrides? test could also be a verb in this context.
Do we really want to override the existing config_directories in the else case?
I'm wondering whether this should be include_once here.
Let's skip this here.
Just wondering why we can't use a boolean here, or something else? Y|N feels really confusing.
Contains ...
empty line
Some kind of description for the class would be nice.
sorry, there are some unneeded spaces here :)
But we refresh them anyway?
Oh I don't see the code which tries to login again.
Comment #12
chx CreditAttribution: chx commentedAlso, 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.
Comment #13
jibraninterdiff from 10-12.
Comment #14
chx CreditAttribution: chx commentedComment #15
BerdirLove this.
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...
Usual nitpick, should be Contains \Drupal\... ;)
Comment #16
chx CreditAttribution: chx commented> 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.
Comment #17
alexpottre moving global $databases to settings... #1951216: Replace global $databases with settings()->get('databases')
Comment #18
chx CreditAttribution: chx commentedFixed nitpick. This should be ready.
Comment #19
alexpottI 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...
Should be
'Drupal\Component\Utility\Settings'
This should be set to FALSE and @var bool
I think we need to qualify what "some" is... perhaps:
Overrides low level and environment specific configuration.
This is a bit bikesheddy but more precise...
Tests the upgrade path without prior creation of config directions.
Should be
Asserts
Comment #20
chx CreditAttribution: chx commentedComment #21
alexpottThis is ready to fly.
Comment #22
effulgentsia CreditAttribution: effulgentsia commentedWow. The interaction here between conf_path() and _drupal_load_test_overrides() is quite a mind bender, but it does accomplish what it needs to.
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.
Comment #23
chx CreditAttribution: chx commentedReally 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.
Comment #24
webchickHm. This smells like a patch it'd be good to have catch look at.
Comment #25
chx CreditAttribution: chx commentedNote 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.
Comment #26
catchCan 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.
Comment #27
ParisLiakos CreditAttribution: ParisLiakos commentedfixed comments and conditional
Comment #29
chx CreditAttribution: chx commentedYou 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.
Comment #30
xjmThis 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.
Comment #31
effulgentsia CreditAttribution: effulgentsia commentedOnce xjm posts the tweaks she's working on, I'll post some additional tweaks as well.
Comment #32
effulgentsia CreditAttribution: effulgentsia commentedComment #35
xjmFiled #1960764: The return value of conf_path() should be named $conf_path.
Comment #36
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.
Comment #37
chx CreditAttribution: chx commentedLet me leave this here.
Comment #38
xjm:)
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.
Comment #39
xjmIt 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.
Comment #41
effulgentsia CreditAttribution: effulgentsia commentedThis 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.
Comment #43
webchick#41: bootstrap-1949724-41.patch queued for re-testing.
Comment #45
effulgentsia CreditAttribution: effulgentsia commentedComment #47
effulgentsia CreditAttribution: effulgentsia commentedComment #48
chx CreditAttribution: chx commentedJust 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.
Comment #49
webchickOverall, this is much more understandable. Thanks!
Only if there's another re-roll... those need a @file. Can also fix on commit.
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).
Comment #50
effulgentsia CreditAttribution: effulgentsia commentedBareMinimalAnonymousUpgradePathTest 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.
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.
Comment #51
effulgentsia CreditAttribution: effulgentsia commentedAssigning 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.
Comment #52
chx CreditAttribution: chx commentedassertSessionKept 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.
Comment #53
chx CreditAttribution: chx commentedI am fine with the changes so when xjm is done with the changes I will re-rtbc
Comment #54
xjmFor #49, I think we could probably call the method finishUpgradeSession() or something?
I'll post another patch shortly.
Comment #55
xjmJust a few notes for myself:
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.
I still feel like this part is a bit fragile. The first time we call
conf_path()
indrupal_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 becausesettings()
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 (plainstatic
, notdrupal_static()
), or checking to see if at least onesettings.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
todrupal_settings_initialize()
would also not go amiss.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."
I'll remove the word "Otherwise."
low-level, environment-specific
Wrapping too early. Also, missing the Oxford comma.
This parameter needs a datatype declaration.
Comment above here would not go amiss.
Missing Oxford comma again. :)
Add @file.
Blank line needed between the class and its first member.
Comment: "Override $update_free_access in settings.php to allow the anonymous user to run the upgrade."
Comma splice. :) The comment could also be a little clearer. I'll fix.
And, I'll rename this.
Another missing
@file
.Similar comment would be good.
Comment #56
xjmComment #57
Berdir13.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.
Comment #58
chx CreditAttribution: chx commentedThanks everyone for working on this! Awesome work.
Comment #59
Damien Tournoud CreditAttribution: Damien Tournoud commentedI 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.Comment #60
xjmYeah, I'd agree about it being brittle. How much would we need to do to change it as you suggest?
Comment #61
chx CreditAttribution: chx commentedI 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.
Comment #62
xjmMaybe 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?
Comment #63
webchickYeah, 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
Comment #64
xjmComment #65
Damien Tournoud CreditAttribution: Damien Tournoud commentedThe win is to not introduce a backward dependency between settings and config, which would make an already messy bootstrap process even messier.
@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:Comment #66
effulgentsia CreditAttribution: effulgentsia commentedI 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.
Comment #67
Damien Tournoud CreditAttribution: Damien Tournoud commentedThat'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.
Comment #68
BerdirA 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.
Comment #69
chx CreditAttribution: chx commentedYes, 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
Comment #70
Damien Tournoud CreditAttribution: Damien Tournoud commentedOk. Then how does passing the settings overrides directly inside the request header (in serialized form) sound?
Comment #71
chx CreditAttribution: chx commentedBoth 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?
Comment #72.0
(not verified) CreditAttribution: commentedUpdated issue summary.