Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem
- Major bugs like #1170362: Install profile is disabled for lots of different reasons and core doesn't allow for that are impossible to reproduce, since Simpletest uses an entirely different installation routine for Drupal.
- The (non-interactive) Drupal installer is not tested at all.
Goal
- Improve Drupal core's test coverage and reduce/eliminate differences between a site installed via Simpletest vs. manual installation.
Details
- Originally, this was done for performance reasons. Which means that we implemented performance optimizations into the custom Simpletest installation routine, instead of improving the performance of the non-interactive installer.
- The Drupal installer got heavily improved meanwhile. It supports a
non-interactive
flag now, which shortcuts most installation steps. - Instead of a different, custom installation procedure,
WebTestBase::setUp()
should just call the non-interactive installer.
Proposed solution
- Replace
WebTestBase::setUp()
's installation routine with the regular, non-interactive Drupal installer.
Dependencies / spin-offs
Changes in this patch, which should be backported:
- #1679570: TestBase does not always use the correct database connection for handling assertions
- #1679594: node_requirements() breaks Drupal installer
- #1688036: Session regenerate and destroy functions do not adhere to drupal_save_session()
- #1688016: drupal_cron_run() unconditionally re-enables writing of the user session
Related issues
- #1297136: Use DrupalNullCache for the installer / #1082328: Provide a proper no-op cache.inc
- #630446: Allow SimpleTest to test the non-interactive installer — The interactive installer cannot be tested currently, because the installer contains an explicit check for the HTTP user agent and denies further execution on a positive match. This happens for security reasons, because anyone would be able to install unlimited numbers of testing sites on all production sites on the net. That's a very complex topic on its own, so the discussion on the interactive installer should ideally stay on that issue.
- #1052692-51: New import API for major version upgrades (was migrate module)
Comment | File | Size | Author |
---|---|---|---|
#119 | 1215104_we_love_php.patch | 529 bytes | chx |
#115 | test.setup_.115.patch | 22.27 KB | sun |
#115 | interdiff.txt | 642 bytes | sun |
#109 | test.setup_.109.patch | 22.27 KB | sun |
#109 | interdiff.txt | 1.05 KB | sun |
Comments
Comment #1
Anonymous (not verified) CreditAttribution: Anonymous commentedi like this concept. even if this adds overhead, tests are stupidly slow right now anyway, so we might as well at least have consistency if we can't have speed.
Comment #3
David_Rothstein CreditAttribution: David_Rothstein commentedSubscribing.
I am trying to decide if this is a duplicate of #630446: Allow SimpleTest to test the non-interactive installer or a prerequisite. Especially since this is filed as a bug report (which I kind of agree with), I'm leaning towards prerequisite :)
Comment #4
dealancer CreditAttribution: dealancer commentedThe sollution is to allow some tests to install Drupal with non-interactive flag, while other tests should go as it is now.
This will make almost all tests to rum faster, while other will user normail site installer.
Comment #5
sunInitial attempt.
This patch throws a kinda weird fatal error when trying to run a test:
Essentially, "Class 'Database' not found in _registry_check_code()" - i.e., in the very function that's supposed to auto-load classes ;)
EDIT: Caused by: #1679594: node_requirements() breaks Drupal installer
Comment #7
David_Rothstein CreditAttribution: David_Rothstein commentedTo deal with the performance issue, maybe it's possible to keep some version of
$this->preloadRegistry()
around? I guess that would require convincing the Drupal installer not to balk if the registry tables already exist when it tries to install, but it seems like that might be doable.Also cross-linking this issue with my comment at #1052692-51: New import API for major version upgrades (was migrate module)... If we can get install_drupal() working inside the context of an already-running site, it would probably help that issue also.
Comment #8
sunIn light of and compared to all of the Testing system improvements I'm currently working on, that preloadRegistry() hack looks like a negligible and poor premature performance optimization to me.
My goal is to streamline and actually "de-hack" the entire testing environment. Only once that is done, we can look into additional performance optimization hacks (on top of the huge list of improvements) - if, and in case that's still necessary then after all.
Comment #9
Anonymous (not verified) CreditAttribution: Anonymous commentedas the author of the preloadRegistry() hack, i say kill it. kill it dead.
Comment #10
Anonymous (not verified) CreditAttribution: Anonymous commentedoh i forgot - when we put that in, the registry was still scanning the known universe, and building entries for functions etc.
that's no longer true, so it's no longer the gain it once was.
Comment #11
sunThe new classloader resolves issues like in #5. :)
Re-rolled against HEAD. This blows up my local web server, so intentionally no .patch suffix.
Comment #12
sunIf I get the error right: The installer fails, because the batch of the Drupal installer is mixed into the SimpleTest batch of the parent site (executing the test).
Batch in a batch, anyone?
Comment #13
lucascaro CreditAttribution: lucascaro commentedWhat about doing a fake batch call if we detect other batch jobs when installing?
something like should do it but it's a hacky hack:
(in function install_run_task($task, &$install_state) )
What do you think? too ugly?
Comment #14
sunThis one should work.
Comment #16
sunoh my. PIFR doesn't like changes to the Drupal installer... :(
Comment #18
sunAttached patch makes both run-tests.sh and the Simpletest UI work -- by backing up Simpletest's batch before invoking install_drupal() and restoring it afterwards.
The only remaining issue is that the Simpletest batch does not redirect to the test results page, even though it finishes successfully. It looks like something in the installer destroys that info (or alternatively Simpletest's batch ID/record).
Comment #20
sunEven when completely skipping Batch API for the non-interactive installer, the batch-finished page of the Simpletest batch still redirects to the front page.
The reason for this is that Batch API validates that the requested batch ID belongs to the current user via drupal_get_token() and that relies on
- the session ID
- the site's private key
- the global $drupal_hash_salt / $databases
The token generated after test execution is different to the stored/expected one.
This means that install_begin_request() or something else in the installer and/or WebTestbase changes too many environment variables that affect the token generation.
I tried to backup + reset pretty much everything already, with no luck.
(all attempts are still in this patch)
Comment #21
sunExtracted the TestBase database connection fixes into #1679570: TestBase does not always use the correct database connection for handling assertions
Extracted the node_requirements() fix into #1679594: node_requirements() breaks Drupal installer
Comment #23
lucascaro CreditAttribution: lucascaro commentedHey @sun I was debugging the installation process for a while and I think I know where the problem is:
The postSave function for UserStorageController does:
And that last drupal_session_regenerate is what is breaking the session id and therefore, changing the token. The call stack for this is:
So basically, using drupal_install, calls install_configure_form_submit() which in turn creates a new account and does:
and destroys the session.
Technically, we could overcome this problem if we temporarily changed $GLOBALS['user']->uid or, even better, if we could set $entity->original->pass to $entity->pass.
We need to figure out if we really need to regenerate the session when creating the user on the installation process, and then we could patch install_configure_form_submit() or maybe doing some juggling with $GLOBALS['user']->uid in WebTestBase::setUp()...
Comment #24
sunI'm sorry, but this needs to be stated:
@lucascaro++
Seriously, THANK YOU for stepping through the debugger for this! :)
That's incredibly helpful. That said, I'm not yet sure why my attempts to backup + restore the current user's session(_id) + stuff didn't have an effect (though possibly, all of that needs to be backed up + restored like $this->originalUser is in the master TestBase::setUp() and ::tearDown(), instead of right after install_drupal()...)
Comment #25
lucascaro CreditAttribution: lucascaro commented@sun no problem!
I think that drupal_session_regenerate() invalidates the old session id in the database:
so maybe the only option is to avoid that session regeneration?
I will try a few options and report back.
Comment #26
lucascaro CreditAttribution: lucascaro commentedHey, wait... this is using the prefixed database so the session should be still working for the batch... *gets confused by simpletest* right?
Comment #27
lucascaro CreditAttribution: lucascaro commentedOk, quick update, session_id with parameters can only be used before session_start so it would not work for restoring the session id. so there are 2 options:
* Updating the session id for the old session in the database, and the token for the batch process or
* Avoid regenerating the session by setting the original password in the installer.
I'd go with the second option if this does not represent a problem for other reasons, since technically we're not changing the user's password (and I think we can get away with not regenerating the session when installing a new site, right?)
Comment #28
lucascaro CreditAttribution: lucascaro commentedWell, adding
before
worked but there's another problem, with user_login_finalize() which does drupal_session_regenerate(); If I disable that session regenerate, the batch process works but I'm afraid it could break the installation process...
So back to restoring stuff on WebTestCase.php, probably is the best option, but we need to update session ids and tokens for batch processes.
Comment #29
lucascaro CreditAttribution: lucascaro commentedThere's also the problem of the cache that generates the failure in the tests above (http://qa.drupal.org/pifr/test/299058).
This is failing because on install, the cache class is set to InstallBackend which is a fake cache with dummy functions and does not save anything in the cache.
This class gets saved in the static $cache_objects in cache() (cache.inc: 26) and since this is not using drupal_static "because we do not want to change the storage of a cache bin mid-request" it keeps using the fake cache even after resetting the cache_classes variable.
@sun: do you think it is possible to perform the installation unsing $this->drupalPost('install.php') or something like that? maybe running though all the installation steps and clicking on the submit button?
This way we would be doing new requests and we would solve the session/batch problem as well as the cache issue...
Comment #30
moshe weitzman CreditAttribution: moshe weitzman commentedNon-interactive install is a feature of Drupal that merits testing just like interactive install. It is heavily used by drush users, jenkins jobs, etc. The title of this issue suggests that we favor exercising one install method over the other.
Comment #31
lucascaro CreditAttribution: lucascaro commented@moshe weitzman indeed, although in this case we're not testing the installation process itself but trying to install drupal to run tests, but I see your point.
I think the issue here is that simpletest is using a completely different method to install drupal, reproducing a variation of the install process that in some cases does not work like neither the interactive install or the non interactive install, so that's what needs to be fixed.
Comment #32
lucascaro CreditAttribution: lucascaro commentedOh, also, using drupalGet wouldn't work because it needs the database.
Comment #33
lucascaro CreditAttribution: lucascaro commentedIt seems to me we need (at least) this change:
from #1297136: Use DrupalNullCache for the installer (originally at #1082328: Provide a proper no-op cache.inc) to make the cache work and then something like the attached patch would work (let's see what the testbot says).
I've also had to add
Before calling _install_module_batch to avoid exceptions and solved the batch problem regenerating and updating the tokens in the database, doing on startUp() (at the very beginning):
and then at the end of tearDown():
This obviously needs a lot of cleanup but we need to come up with something that works first ;)
So far this is passing my small subset of tests and I'll let testBot decide whether this passes or not all of the tests for core :)
Comment #34
lucascaro CreditAttribution: lucascaro commentedComment #36
lucascaro CreditAttribution: lucascaro commentednow with support for non-batched tests! (tested with drush test-run)
Comment #38
sun@moshe weitzman: It's not clear to me what you tried to say. This issue is about replacing Simpletest's custom code for installing the Drupal site (under test) with the regular non-interactive Drupal installer. The Drupal installer is not tested at all currently.
Please note that testing of the interactive installer is left for #630446: Allow SimpleTest to test the non-interactive installer. It cannot be tested currently, because the installer contains an explicit check for the HTTP user agent and denies further execution on a positive match. This happens for security reasons, because anyone would be able to install unlimited numbers of testing sites on all production sites on the net. That's a very complex topic on its own, so the discussion on the interactive installer should ideally stay on that issue.
Thus, the interactive installer cannot be used for this issue (and also should not be used, since the internal browser is considerably slower than the direct script execution).
@lucascaro: Thanks for pushing this forward! I'll be away the next days, but will try to review your changes as soon as possible.
Comment #39
moshe weitzman CreditAttribution: moshe weitzman commentedAh, OK. I misunderstood the issue. Removing the first part since it is too vague (different from what?).
Comment #40
sunWell, just different. :) It's an entirely custom installation procedure that shortcuts and redoes each and everything that the installer normally does with regard to requirements, install profile, module installation, and initial setup. It is randomly calling into some functions, and hopes/assumes that the result is going to be "similar" to the one of the regular installer. That's different. ;)
Attached is an interdiff between #20 and #36.
Given that we found the actual cause and reason for why the batch (token) gets corrupted, I guess that all the changes to install_run_task() for shortcutting the non-interactive/non-progressive batch case can be reverted.
Most of the remaining test failures are still cache related. The cause for that is relatively clear: install_begin_request() sets/overrides global $conf options to enable the NullBackend. This means we need to backup $conf before the installation and restore it afterwards.
I'll quickly try to do those adjustments.
Comment #41
sunSo... apparently, we already have a mechanism for this, which is drupal_save_session(FALSE).
The problem is that drupal_save_session() is not checked for the session regenerate and destroy cases.
I'm totally not sure whether this is valid and legit, but when injecting the identical condition that happens in the session commit and other callbacks into the regenerate and destroy callbacks, then this patch magically starts to work! :)
Perhaps @moshe weitzman can tell us whether this is utterly wrong or even a bugfix of its own...? :)
#23 explains that the Drupal installer is saving the $user for uid 1 to the (new) database, and is automatically logged in as the last step of the installation process. While everything happens in the child site under test, the PHP session handling happens in the global scope. Therefore, this $user->save() as well as the user_login_finalize() regenerate/destroy the session ID + session + cookies of the user that is running the test through the Simpletest UI. That, in turn, changes the value of drupal_get_token(), which Batch API saves and validates for each batch to be executed.
I'll attach the full patch including all debugging comments and stuff so far + interdiff to this comment, and clean up and reduce the whole thing to the required changes in my next comment (so we can get back to this version, in case these session.inc changes are invalid).
Comment #43
sunAnd here's the cleaned up patch with proper code and comments and stuff. :)
Comment #45
sunFor the record: This patch actually passed. The remaining test failures and fatal errors are actual errors in HEAD.
Comment #46
sunShould fix all fatals.
Comment #47
sunMoved the drupal_save_session(FALSE) call into TestBase::prepareEnvironment(), which is where the global $user is backed up. It is re-enabled in TestBase::tearDown(), so it makes sense to keep all of that in TestBase.
Comment #49
sunAttached patch should hopefully pass.
547b963 Fixed form_process_checkboxes()/form_type_checkboxes_value() hack.
a4e1f9f Fixed full revert of global $conf breaks rebuild operations that depend on the (default) theme.
Comment #51
lucascaro CreditAttribution: lucascaro commentedHey @sun that's almost there, that one missing test seems to be related to drupal_save_session and, while debugging, I found that it's reset to TRUE in drupal_cron_run. See stack trace:
core/includes/common.inc.drupal_cron_run:5279 calls
and is executed after the drupal_save_session(FALSE) in TestBase. The thing here is that prepareEnvironment is being executed before install_drupal
When I re add drupal_save_session(FALSE) below install_drupal, this doesn't fail anymore. Here's a patch with this change.
Comment #52
lucascaro CreditAttribution: lucascaro commentedThe last patch had some garbage from my tests, sorry about that. Here's the same patch without the test files.
Comment #53
sunoh, wow - drupal_cron_run() is Doing It Wrong™ ! ;)
drupal_save_session() contains a pointer to the handbook page http://drupal.org/node/218104 that explains how to use it properly (which, alas, should really be documented on the API function instead).
026de92 Fixed drupal_cron_run() resets drupal_save_session().
Comment #53.0
sunRewrote the issue summary.
Comment #53.1
sunUpdated issue summary.
Comment #54
sunRewrote the issue summary and extracted the following two changes into own issues:
#1688036: Session regenerate and destroy functions do not adhere to drupal_save_session()
#1688016: drupal_cron_run() unconditionally re-enables writing of the user session
Comment #55
lucascaro CreditAttribution: lucascaro commentedJust an update, here's a work in progress attempt to port this patch to D7.
Comment #56
sunRe-rolled against HEAD.
Still waiting for the dependencies to get committed. (see issue summary)
Comment #57
sunAll of the dependencies landed! :)
Comment #58
sunThat said, I wonder whether the block schema change is actually required for this patch to pass, or whether that was caused by some other hiccup only.
In case this patch passes, we most likely should consider to do that schema change nevertheless -- but in a separate issue.
Comment #59
moshe weitzman CreditAttribution: moshe weitzman commentedNice code cleanup. No UI impact.
Comment #60
sunThis patch essentially tests the Drupal installer (implicitly) with every web test case.
Due to that, I'd ideally like this patch to go in before #1599108: Allow modules to register services and subscriber services (events) -- as it's entirely not clear to me at this point, whether the kernel patches are taking the Drupal installer properly into account.
For full disclosure though I need to amend that this patch slows down the total time the testbot needs for the Drupal core suite by ~3 minutes. However, that only means we should work on the installer's performance — there are a lot of optimization possibilities in that code, and doing so will not only benefit the test suite, but also every single Drupal site that is being installed.
Comment #61
aspilicious CreditAttribution: aspilicious commented#58: test.setup_.57.without-block.patch queued for re-testing.
Comment #63
sunMerged HEAD.
Comment #65
sunmeh. Exactly what I guessed:
:-(
Comment #66
effulgentsia CreditAttribution: effulgentsia commented#1719488: Rename language_manager() to language() and related cleanup might fix that.
Comment #67
sun#63: test.setup_.63.patch queued for re-testing.
Comment #69
sunMerged in HEAD.
Comment #71
sunResolved the new test failures thanks to the help of @effulgentsia!
Fixed language() tries to access language.manager service outside of request scope.
Fixed t() attempts to invoke locale() even if Locale module is not installed.
Comment #73
sunAlright, the remaining test failures seem to be two-fold:
1) update.php requires to override drupal_container() in the same way as install.php/install_begin_request() is doing (but there's not even a comparable function/entry-point, so it's going to be hard to figure out the proper place for A) applying the override and B) reverting it as soon as that is possible).
2) Many unit tests seem to be failing. Because test assertion methods are calling into t(). So my suggested fix for t() in #71 might not be correct, as it adds a check for
module_exists('locale')
into t() in order to check whether A) the locale() function exists and B) whether its database tables exist. The call to module_exists() triggers calls to system_list(), which tries to query the database for enabled modules, but unit tests do not have a database.Comment #74
Crell CreditAttribution: Crell commentedIt was recently decided to remove t() from assertion messages anyway: #500866: [META] remove t() from assert message
So that problem itself isn't a blocker here, but committing that patch arguably is.
Comment #75
moshe weitzman CreditAttribution: moshe weitzman commentedThe hard coded string array in settings.php works regardless of locale module. So it is untrue that t() does nothing if locale is disabled.
Comment #76
bforchhammer CreditAttribution: bforchhammer commentedComing from #1738330: Confusing Language negotiation when accessing / it would be nice if it was possible to change setup parameters like the langcode... looks like that's not possible in patch #71 at the moment. So maybe we can move the
$settings
variable to a class field, or add a respective parameter tosetUp()
?Comment #77
bforchhammer CreditAttribution: bforchhammer commentedSorry, didn't mean to change the status.
Comment #78
sun78a) Fixed module_list() and module_load_all() are reset with every drupal_static_reset(), breaking unit tests.
78b) Reverted module_load_all() $has_run flag to drupal_static(), since only module_list() cannot use drupal_static().
Comment #80
sunTurns out the entire upgrade path to D8 is b0rked!
When Locale module was enabled in D7, then update.php ends in an error, because update_prepare_d8_bootstrap() is invoked with all modules enabled and loaded, and t() is being called deeper in there, and because Locale module exists but the database tables haven't been fully upgraded yet, the updater ends in an error. The module list is reduced to system.module only slightly later in update.php when the actual updates are being executed.
All of this got revealed due to the replacement of drupal_static() with a plain static in module_list() in this patch, which is required, because otherwise any call to drupal_static_reset() in a unit test would reset that module_list() static and thus, the next call into it would attempt to retrieve the list of enabled modules via system_list() from the database -- but there is no database for unit tests.
After fixing that - and also removing the suppression of errors in update.php - further testing of LanguageUpgradePath revealed that various module update functions are calling into module API functions, which should not be available at all in the first place. Thus, also fixed those module updates.
Revert "Reverted module_load_all() $has_run flag to drupal_static(), since only module_list() cannot use...
Fixed module list in update.php and upgrade path to D8.
This one should come back green.
Comment #82
sun...adding this to my Top 10 of Most Horrible Commit Messages Ever™:
Fixed update.php requires full module list for certain operations, but not for the actual update environment.
Comment #84
sunFixed t() tries to invoke locale() during update.php requirements checks.
(also added docs to t() for this clusterfuck)
Comment #85
sunGREEEN! :)
Final tweak:
Performance: Make t() check for the primary locale() condition first.
Comment #86
sunI'm checking whether update.php upgrade path fixes could be extracted into an own issue, or whether they depend on the module_list() + drupal_static() changes that are required for this patch.
Comment #87
sunExtracted the update.php upgrade path fixes into this long-standing epic:
#922996: Upgrade path is broken since update.php does not enforce empty module list
Comment #88
aspilicious CreditAttribution: aspilicious commentedThis patch has 500 test passes LESS than the referenced one in the last comment. Thats weird....
Is it possible some tests don't get run, or is there something else going on?
Comment #89
sun#85: test.setup_.85.patch queued for re-testing.
Comment #91
sunMerged in HEAD.
Comment #92
sunThere's some pushback on the update.php changes that have been extracted into #922996: Upgrade path is broken since update.php does not enforce empty module list
So let's not hold up this patch on that. This means that we need to revert those changes and "unfix" the LanguageUpgradePathTest, which likely means that we have to implant a stop-gap fix somewhere to temporarily make it pass the tests. I'm not sure where exactly that would have to happen, but we need to figure it out.
Comment #93
sunReverted update.php and upgrade path changes.
Comment #95
sunMerged in HEAD.
Comment #96
sunTurns out that the adjustments to module_list() + module_load_all() + t() that happened later here effectively make this patch pass the tests already.
Thus, no further changes are required to work around the LanguageUpgradePathTest failures for this issue. RTBC? :)
Comment #97
effulgentsia CreditAttribution: effulgentsia commentedJust a reroll for HEAD changes in TestBase::tearDown().
Comment #98
effulgentsia CreditAttribution: effulgentsia commentedJust some questions: none of it necessarily warrants a "needs work".
I don't understand why this got moved into the
if ($batch['progressive']) {
block. There might be an obvious reason, but I don't know the Batch API well enough to say whether this makes sense."Drupal is only happens to be" doesn't sound quite right.
Comment says to do this in case it's the last task, but it's being done with no if statement. What if it's not the last task? Does removing $install_state cause problems then?
Comment says this is important for security, but we're now limiting it to interactive mode only. Are we sure we don't need this security in non-interactive mode?
Why is this being removed?
Comment #99
sunThanks for the review! :)
If you check the old code flow closely, then you see that it tries to access
$batch['id']
after the line that set$batch = NULL;
I stepped through the Batch API code and the batch ID only seems to be registered in the session for progressive batches.
Yes, the non-interactive installer can only be executed if you have access to execute PHP on the server. Also, all that code is doing is to check for a certain HTTP header, which obviously doesn't exist in the first place when executing the non-interactive installer.
drupal_flush_all_caches() is calling drupal_static_reset() already and is invoked right after this line, so the additional static reset is needless.
Adjusted for the other issues:
Fixed grammar in drupal_installation_attempted().
Added installation_finished condition before removal of global $install_state.
Comment #101
sungeez, HEAD is moving fast... (and it was one of my own patches that caused conflicts :P)
Comment #102
effulgentsia CreditAttribution: effulgentsia commentedThanks.
Comment #103
sun#101: test.setup_.101.patch queued for re-testing.
Comment #104
sun#101: test.setup_.101.patch queued for re-testing.
Comment #105
catchThis is massively changing the meaning of $locale_exists. It's allowably to have a different module that implements the locale() function without locale module being enabled. That's a weird hack that is incredibly outdated, but if we have that feature we can't just lose it here.
Also:
Might be a case for the drupal_static_fast pattern?
We just got rid of this in #1503224: Cleanup module_list() :(
Comment #106
Gábor HojtsyWhile theoretically another module could implement locale() it was also possible in Drupal 7 to swap in the cache array with an arrayaccess implementation and skip most of the logic in locale() by feeding the data there. I have not looked at the updated cache backend for locale() but it is likely still swappable in some way now.
Comment #107
sunThe review issues on the t()/locale() change looked impossible to resolve for me, since we're calling into an API function without any kind of protection and check for whether that API is actually ready to operate, and it can't even be controlled through any kind of variable. But PHP code cannot be unloaded. So locale() is called unconditionally, just because it has been loaded at some point.
However. Since it seems like we need to retain that weird magic function "feature" as-is, and since we cannot unload PHP code, I simply turned it around, and put the the check into the locale() function. This means that the locale() function needs to make sure on its own that it is being called in an environment in which the (Locale) module is actually enabled (and thus its database tables are installed).
Use advanced drupal_static() pattern for cache().
Moved Locale module installation check from t() into locale() itself.
Comment #109
sunFixed locale_reset() relies on 'locale' drupal_static name.
Comment #110
sunI think the latest patch addresses the concerns of #105 as much and as cleanly as possible.
Alternative "implementations" of locale() will only have to follow those changes, if they are used in tests. (or more precisely: if the parent-site/test-runner has the alternative implementation loaded and intends to run tests)
The only part I did not address yet is:
Yes. It's pretty clear to me by now that the removal of $reset arguments from module_list() & co was wrong, since it allows a potentially fixed module list to get (unintentionally) reset, which not only affects tests but also special front controllers like the installer and update.php. However, there's hope that #1331486: Move module_invoke_*() and friends to an Extensions class will allow us to replace the entire module/extensions service with a specialized LockedExtensions implementation, so we'd get rid of $reset arguments there and all of this could be simplified later on.
Comment #111
sunTentatively moving back to RTBC, since the only concerns since last RTBC have been addressed.
Comment #112
lucascaro CreditAttribution: lucascaro commented+1 to RTBC
Comment #113
Gábor Hojtsy@catch says the cache clearing techniques here might help with solving #1787520: Translations not imported on installation as well which is also about a module being enabled but the fixed module list kept intact and therefore locale not exposing its stream wrapper which would be needed for importing .po files in the installer.
Comment #114
catchStill locale_exits.
Comment #115
sunoh. Sorry, that detail wasn't clear in the original review.
Comment #116
lucascaro CreditAttribution: lucascaro commentedJust in case I've re tested installing using
drush si
and interactively using the browser. It all seemed to be working fine both before and after applying the patch. I've also ran the tests using the UI (the only other thing that is not tested by the bot, I think) and it works.Comment #117
catchThanks! Committed/pushed to 8.x. Bit of luck this will unblock some other issues.
Comment #118
lucascaro CreditAttribution: lucascaro commentedawesome! is this worth backporting to D7? I started working on that a while ago but there have been some changes along the way, and @sun mentioned it could be tricky... I'd like to try anyways unless you guys think it can't or shouldn't be done.
Comment #119
chx CreditAttribution: chx commentedOpsie. Let me quote: http://php.net/unset
Comment #120
tim.plunkettRTBC +1, just for process's sake.
Comment #121
xjmI'd ah... hesitate to backport this.
Comment #122
webchickCommitted and pushed the follow-up in #119.
For those to whom #119 didn't make sense, here's what chx had to say when I asked him about the patch:
"webchick: when it finishes install it doesnt unset the install_state so the system thinks it's still installing
webchick: for example, drupal_get_profile is broken (at least)"
Comment #123
chx CreditAttribution: chx commentedOK, so to follow up on how I found this: after a test method is torn down, drupal_get_profile still reports the testing profile because install_state lingers. Any other code that peeks at install_state would be similarly broken. And it's particularly hard to write a test for something that only happens outside of a test...
Comment #124.0
(not verified) CreditAttribution: commentedUpdated issue summary.