Problem
- In particular since #1961938: Test the interactive installer, we have a very unhealthy amount of Simpletest environment overrides and global state hacks, which is unmaintainable.
Goal
- Eliminate the entirety of Simpletest hacks and overrides. Replace them with native multi-site functionality.
- Each web test gets an actual site in
/sites/simpletest/$prefix
. - For WebTests, we simply let the installer do its job as usual.
- The site and test environment is negotiated only once, by
conf_path()
, viadrupal_valid_test_ua()
. drupal_valid_test_ua()
is adjusted to be able to operate without having access tosettings.php
, so as to remove a circular dependency in the early installer (install_begin_request()
).
Anatomy
TestBase::prepareDatabasePrefix()
additionally prepares a site directory:/sites/simpletest/$prefix
TestBase::setUp()
(new) primesdrupal_valid_test_ua()
with the test prefix and resetsconf_path()
.conf_path()
usesdrupal_valid_test_ua()
to force-override the site directory to/sites/simpletest/$prefix
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 ownsettings.php
file.- To allow
conf_path()
to usedrupal_valid_test_ua()
in super-early bootstrap, we no longer rely on$drupal_hash_salt
insettings.php
, but instead, a test-prefix-specific$private_key
is read from a new/sites/simpletest/$prefix/key.php
file (written bydrupal_generate_test_ua()
). - Due to the nested site subdirectory (
simpletest/$prefix
), a test site directory is not a valid regular site directory, and thus not discovered byfind_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
- The site discovery process (
find_conf_path()
) does not support nested subdirectories. The/sites/[simpletest]/12345
directories are not considered at regular runtime. - 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.) - 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. - Each inbound HTTP request to Drupal
- checks whether User-Agent HTTP header contains the simpletest prefix
- if it does, the private key is read from the corresponding
/sites/simpletest/$prefix/.htkey
file in the filesystem (hard-wired) - only if the private key can be read
- only if the given untrusted HMAC can be validated using the private key
- 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 insettings.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 whichsettings.php
is going to be loaded for a test (in super-early bootstrap). - 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. 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.-
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.
- 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
- Developers that want to run tests need to create a
/sites/simpletest
directory and make it writable. - None otherwise.
Dependencies
- #2170023: Use exceptions when something goes wrong in test setup
- #1376122: Stream wrappers of parent site are leaking into all tests
- #2176141: Add a return value to file_save_htaccess()
- #2176131: Database configuration form in installer still uses 'db_prefix' instead of 'prefix'
- #2176151: rebuild.php does not include /core/vendor/autoload.php
Blocks
Comment | File | Size | Author |
---|---|---|---|
#112 | interdiff.txt | 3.11 KB | sun |
#112 | test.site_.112.patch | 104.5 KB | sun |
#111 | interdiff.txt | 2.21 KB | sun |
#111 | test.site_.110.patch | 103.53 KB | sun |
#108 | 104-108-interdiff.txt | 1001 bytes | alexpott |
Comments
Comment #1
sunComment #2
amateescu CreditAttribution: amateescu commentedCan't we do this automatically when the simpletest module is installed?
Comment #5
larowlan+1000000000
Comment #6
sunHm. Can't make sense of the test failures.
On one hand, PIFR prepares the (entire) checkout directory with:
I.e., all directories should be writable. But yet:
I'm not able to reproduce that permission error locally... (but then again, I'm on Windows)
Comment #7
sunFixed test site directory is no longer writable after executing the installer.
Comment #10
sunFixed 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.
Comment #12
sunMajor update + significant improvements:
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.
Comment #14
sunFixed invalid PHP syntax in simpletest.install.
Comment #15
chx CreditAttribution: chx commentedThis 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?
Comment #17
larowlanSo 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?
Comment #19
BerdirIt 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.
Comment #20
larowlan@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.
Comment #21
larowlan17: test-site-2171683.17.patch queued for re-testing.
Comment #23
sun@larowlan saved my day. Thanks a ton! :)
Comment #25
chx CreditAttribution: chx commentedThis 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?
Comment #26
sun@chx: Sorry for not replying earlier to your question. I'm not able to see what PhpStorage would buy us aside from problems:
PhpStorageFactory
depends onSettings
anddrupal_hash_salt()
→ Circular dependency. If anything, PhpStorage only adds dependencies that are not yet available at the point in time whendrupal_valid_test_ua()
is called.MTimeProtected[Fast]FileStorage
needs a secret key to operate → but what we are storing is the secret key itself./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:
Comment #30
chx CreditAttribution: chx commentedWhile 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.
Comment #31
sun@chx:
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) changeddrupal_generate_test_ua()
to additionally create a .htaccess file that is specific to the simpletest prefix.Comment #33
chx CreditAttribution: chx commented> 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?
Comment #34
sunComment #37
sunInteractive 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!)
Comment #39
chx CreditAttribution: chx commentedThere 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.
Comment #40
sunThis should leave us with LanguageNegotiationInfoTest + MenuRouterTest... (before adding back any security protections)
Comment #41
chx CreditAttribution: chx commentedThere'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...
Comment #43
tstoecklerReview of the interdiff from #37:
We should update the comment to say that during testing there might be no 'config.factory' service.
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...
Yay, this is *sooo* useful. +10000
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;
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 {}.
This is *awesome* but we should explain why, i.e. that you can kill your site if you override this incorrectly.
Comment #44
tstoecklerReview of the interdiff in #40:
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.
"skip unit testing DrupalKernel" I thought that's what the thing is doing? I don't understand that commment.
Minor: Maybe array_filter() is more concise/clear?
Comment #45
sunThis 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.
Comment #47
tstoecklerThe usage of $this->container in tests is discouraged. \Drupal should be used instead.
Yeah, thanks :-)
Comment #48
sun/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).
Comment #49
sunSorry, cross-posted with @tstoeckler...
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.
Comment #50
chx CreditAttribution: chx commentedThis does look good to me. Thanks for keeping Drupal secure even in non recommended scenarios.
Comment #51
sunSorry, 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:
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.
Also added to the issue summary.
Comment #52
larowlanWill definitely use this in our build process by configuring it to use SQLite in /dev/shm
Comment #53
sunI 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
: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! :-)
Comment #54
tstoecklerSo 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?
Comment #55
sunReady 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.
Comment #56
larowlanFirstly, 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.
sneaky: can we reference http://drupal.org/2016629 with this @todo.
This is nice cleanup, production code shouldn't need to be test aware, ideally - so nice.
Is there a url for this @todo?
Can we get a url to the issue which will reinstate this, makes it easier to grep for relevant todos.
nice cleanup
this can go now?
this is a very useful addition
I think this is still useful, but instead of return, another RuntimeException?
can we get an issue link here
and again, issue link for @todos
Pretty sure @catch already created one for the private files path.
more debugging++
Is there anything already available in our vast codebase (including vendor) that already does this?
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.
something we should tackle in the bootstrap cleanup - moving from globals in settings.php to the kernel and then dropping.
nitpick: Needs docblock
Can we get a link here too?
one more todo needing a link (committers seem to be asking for these now)
Need docblocks
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.
how did this pass in head :)
sounds like a missing module dependency in a test? ie a test view uses a plugin from a module that isn't installed?
unrelated?
Comment #57
sunThanks for the extensive review, @larowlan!
(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.
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™.
WebTestBase::translatePostValues()
: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().
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
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
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.
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).
/core/vendor/autoload.php
inrebuild.php
breaks therebuild.php
script with this patch, becausedrupal_bootstrap(DRUPAL_BOOTSTRAP_CONFIGURATION)
now calls intodrupal_valid_test_ua()
, which uses theCrypt
class. Every front-controller script in D8 has to include/core/vendor/autoload.php
first. Therefore, that is a required fix here.Comment #58
larowlanThanks 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 :)
Yes I'm saying I don't think we should remove it, its another measure to prevent tests stuffing your prod db.
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.
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.
Comment #59
tim.plunkettOnly looking at the views stuff:
Why this change?
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.
Comment #60
sunre: #58: @larowlan:
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
Comment #61
sunFinal adjustment:
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
Comment #62
chx CreditAttribution: chx commentedI 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?
Comment #63
sunOK, I've removed all changes that are not strictly required here:
Regarding the other mentioned changes:
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.
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'.
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.
Comment #64
sunSorry, something went wrong in those reversions. I did not mean to delete that code.
Comment #67
chx CreditAttribution: chx commentedLet 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 ?
Comment #68
chx CreditAttribution: chx commentedFiled #2176151: rebuild.php does not include /core/vendor/autoload.php for the vendor_autoload.
Comment #69
chx CreditAttribution: chx commentedPlease disregard everything I said, I already won't fixed the one in #68 and I am unfollowing this issue too.
Comment #70
sunWhile not as complete as here, I managed to additionally extract #2176131: Database configuration form in installer still uses 'db_prefix' instead of 'prefix'
Comment #71
larowlanI think this is ready but we should give security team a chance to respond, will ping a few members on Monday
Comment #72
sunTo 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:
→ blocks #2176141: Add a return value to file_save_htaccess()
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.
Comment #73
sunThanks 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
Comment #76
tstoecklerOops, forgot the patch.
Comment #77
sunThe spin-off issues have much more complete patches now, so we should postpone this issue to get them in first:
I'll re-roll this patch from my sandbox branch once these have been committed.
Comment #80
sunAll except of one spin-offs/dependencies have landed - the last remaining: #2176141: Add a return value to file_save_htaccess()
Comment #81
sunAnd the last dependency is gone too now :-)
No interdiff, since merging HEAD just simply eliminated some hunks.
Comment #82
sunThis 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? :)
Comment #83
moshe weitzman CreditAttribution: moshe weitzman commentedI 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.
Comment #84
sunThanks @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.
Comment #85
larowlanHave reviewed the code (again) and happy with it.
Manual testing now.
Comment #86
larowlanManual testing observations
1) Can no longer enable/install Simpletest module on simplytest.me - see screenshot
2) Testing locally, simpletest UI works fine for a mix of phpunit and web tests.
Comment #90
tstoecklerOops, and this was a human fail. Wanted to re-upload the patch...
Comment #92
sunLooks like yet another random testbot OS failure. Anyway, merged 8.x (cleanly).
Comment #95
sunComment #96
sunMerge branch '8.x' into test-site-2171683-sun
Comment #97
sun96: test.site_.96.patch queued for re-testing.
Comment #99
sunMerge branch '8.x' into test-site-2171683-sun
Comment #100
sunMerge branch '8.x' into test-site-2171683-sun
Comment #101
catchThis change looks unrelated?
Why not drupal_unlink() - same goes for other raw file operations in the patch. If there's a good reason, needs a comment.
Comment #102
bzrudi71 CreditAttribution: bzrudi71 commentedMost 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...
Comment #103
sunAll 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.
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.
Comment #104
sunComment #105
larowlanback to rtbc if green
Comment #106
catchChanges work for me. Committed/pushed to 8.x, thanks!
Comment #107
catchThis 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).
Comment #108
alexpottThe 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.
Comment #109
sunPlease excuse my ignorance, but I'm not able to follow recent comments:
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. :)
Comment #110
alexpott1. 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.
Comment #111
sun@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 fromTestBase::prepareEnvironment()
, becauseWebTestBase::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 onglobal $databases
instead.Comment #112
suninstall_settings_form()
still needs to account forglobal $databases
, in case database connection settings have been prepared insettings.php
already.Comment #114
alexpottHere 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.
Comment #116
alexpott@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.
Comment #117
catchCommitted/pushed #112 to 8.x, thanks!
Comment #118
xjmSurely this needs some change record action? :)
Comment #119
Les LimWrote 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
Comment #120
tstoecklerThanks @Les Lim. I added a short paragraph about the .htaccess protection and published the change notice.
Comment #121
Les Limde-tagifying.
Comment #122
BerdirI 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.
Comment #123
amateescu CreditAttribution: amateescu commentedShould 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..
Comment #124
BerdirStrange, 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).
Comment #125
amateescu CreditAttribution: amateescu commentedYou'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.
Comment #126
longwaveI 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
Comment #127
andypostI see the slowness in testing and talked about it in IRC, so maybe this one caused so filed #2194267: Fix testing speed regressions
Comment #128
BerdirOpened #2194273: Avoid repeated cache tag deletions for the cache tags.
Comment #129
sun*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