Come together with the global Drupal community in Rotterdam, 28 Sept – 1 Oct 2026. Sessions, contribution, connection, and Early Bird savings until 8 June.Problem/Motivation
While I'm opening this as major, as soon as we have a core or contrib update path that fails due to this code, it is going to become critical.
// Replace User 1 with the user created here.
// @todo: do this without saving the user account.
/** @var \Drupal\user\UserInterface $account */
$account = User::load(1);
$account->setPassword($this->rootUser->pass_raw);
$account->setEmail($this->rootUser->getEmail());
$account->setUsername($this->rootUser->getUsername());
$account->save();
We try not to save content entities in hook_update_N() because the schema might be out of sync, or hook invocations might rely on other schemas that also aren't outdated yet.
So our upgrade path test base class definitely shouldn't do it before updates have even run.
Proposed resolution
Any of these two would be better than the status quo:
1. Rely on the database dump having predictable values (admin, admin@example.com, pass) etc. and set $this->rootUser so they match.
2. Directly update the database tables with the values
Currently the MR adopts the proposed solution #2
Remaining tasks
User interface changes
API changes
Data model changes
Beta phase evaluation
| Issue category | Bug because saving an entity prior to running updates has the potential to cause unforeseen issues |
|---|---|
| Issue priority | Major because we need confidence our update tests are as close to reality as possible |
| Comment | File | Size | Author |
|---|
Issue fork drupal-2560237
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
- 2560237-updatepath-testbase-saves-the
changes, plain diff MR !9693
Comments
Comment #2
larowlanI wonder what option three is :)
Comment #3
larowlanWent with option 2, option 1 relies on a known hash salt, which is not a good idea because I think that also means the simpletest cookie/header for the test User-agent could be easier to guess.
Comment #4
jhedstromAt first I thought this may be problematic, but it is a very basic service that isn't going to fire off hooks or anything else that might potentially interfere with the database prior to running updates.
I think the patch in #3 is good to go. I added a beta evaluation.
Comment #5
webchickA bug report implies that we should have test coverage to ensure we don't reintroduce the bug. However, the issue summary seems to be talking about fear of a hypothetical bug, not that something's actually broken, so maybe this is fine?
Comment #6
alexpottWhy wouldn't we do option 1?
Comment #7
webchickComment #8
larowlanEach child site gets it's own hash salt - so that would mean we'd have to hand-edit the database dumps to call out to the \Drupal::service('password') to set the hashed password in the dumps.
This would mean the dump command relies on manual intervention.
Comment #9
jhedstromand
seem like good enough reasons not to go with option #1 in the IS. Some of our partial dump files already use some very light-weight APIs like key value, so calling out the the password hashing service seems ok in this instance.
Comment #10
larowlanFrom
\Drupal\simpletest\TestBase::prepareEnvironmentSo I'm pretty sure that means we can't have the hashed password in the dump in advance.
Setting back to RTBC to get on @alexpott and @webchick's radar (apologies if that's not the right thing to do).
Comment #11
alexpottI think the problem with this change is that is makes all alternate entity storage not upgrade path testable. Going to ping @chx for an opinion.
Comment #12
catch@webchick we could test this by having a test module that breaks something in entity loading and saving until after updates are run.
That same test module could actually break the current approach though depending on what change we do - since the base class would need to be updated for any schema changes too, which is what @alexpott is getting at in #11.
Comment #21
catchI think alternative entity storages could override this method. We've been lucky that this hasn't caused a problem so far, given it's been six years with the code sitting there, moving to a task.
Comment #24
quietone commentedUpdating tag.
Comment #26
smustgrave commentedUpdated it for D10
Since it's a task does it still need a test?
Comment #27
_utsavsharma commentedTried to fix CCF for #26.
Comment #29
ankithashettyTried to fix the phpunit errors by replacing the deprecated method
getUsername()withgetAccountName().Thanks!
Comment #30
smustgrave commentedThanks @ankithashetty and good find!
Comment #32
smustgrave commentedRandom ckeditor5
Comment #33
alexpottI think we could do with a comment to explain why we're doing it like this - so no one thinks this is a good idea to copy and to put into real run-time code.
Comment #37
bhanu951 commentedReroll of #29 and added comment as suggested in #33.
Comment #38
smustgrave commentedCan issue summary be cleaned up some. Appears to have 2 solutions but not highlighted which.
Comment #39
bhanu951 commentedComment #40
bhanu951 commentedUpdated IS. Setting NR.
Comment #41
amateescu commentedLooks ready to me!
This is a blocker for #3363732: entity.last_installed_schema.repository has a race condition.
Comment #42
catchLeft one suggestion on the MR.
Comment #43
alexpottComment #44
alexpottCommitted and pushed 8b53ceb644 to 11.x and a113593f22 to 10.4.x. Thanks!
Comment #47
alexpott