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.In Drupal 8, in the function WebTestBase::setUp() sets up user 1 to have rational values:
// Define information about the user 1 account.
$this->root_user = new UserSession(array(
'uid' => 1,
'name' => 'admin',
'mail' => 'admin@example.com',
'pass_raw' => $this->randomName(),
));
The same is not true in Drupal 7's DrupalWebTestCase::setUp(), which means that during tests, user 1 has the following values:
select uid, name, mail from simpletest508334users where uid=1;
+-----+-----------------------+-----------------------+
| uid | name | mail |
+-----+-----------------------+-----------------------+
| 1 | placeholder-for-uid-1 | placeholder-for-uid-1 |
+-----+-----------------------+-----------------------+
1 row in set (0.00 sec)
In most cases this will go unnoticed, but it is not unreasonable for modules to _assume_ that all users have a well-formed email address.
Proposed solution:
(1) Create a well-formed email address for user 1 during testing, as in D8
(2) Make sure the email address of user 1 is never placeholder-for-uid-1 to begin with, perhaps use placeholder-for-uid-1@example.com instead? (this second point being outside the scope of this issue though, perhaps)
| Comment | File | Size | Author |
|---|---|---|---|
| #15 | core-8.x-user-mail-well-formed-2276491-13.patch | 449 bytes | mgifford |
| #13 | core-8.x-user-mail-well-formed-2276491-13.patch | 449 bytes | adci_contributor |
| #8 | 2276491-8-core-8.x-user-mail-well-formed.patch | 458 bytes | alberto56 |
| #6 | 2276491-6-core-8.x-user-mail-well-formed.patch | 460 bytes | alberto56 |
| #2 | 2276491-2-core-7.x-user-mail-well-formed.patch | 464 bytes | alberto56 |
Comments
Comment #1
alberto56 CreditAttribution: alberto56 commentedComment #2
alberto56 CreditAttribution: alberto56 commentedActually this is not only in simpletest. If for any reason the user 1 address is not entered, the default email is 'placeholder-for-user-1', which is not well-formed.
Here is a patch which fixes this.
For those wondering why this can be important, here is my workflow:
I have a simpletest which fail, but the workflow to recreate the fail is very complex, including creating many users, nodes, etc.
To debug, I just insert die() before the first fail in my test, run my test, and then change the database prefix in local dev site's sites/default/settings.php.
I have a module installed which forces me to fill in a bunch of fields before I can use an account for anything, so when I next fill in my site, i fill in all the fields, but when I save I get the message placeholder-for-user-1 is not valid, which is true, but there is no field allowing me to change it.
Comment #3
sunDid you grep the code base for that placeholder string to ensure there is no code that uses it as condition? Grep results would be helpful.
I checked
Drupal\Core\Installer\Form\SiteConfigureForm::submitForm(), which is responsible for updating the placeholder with the real values. It does not use a condition (aside from the uid condition).Comment #4
alberto56 CreditAttribution: alberto56 commented@sun thanks, here is the result of the grep (once the patch is applied). It seems that this is not used anywhere else as far as I can tell.
Waiting to see what the testbot thinks.
Cheers,
Albert
Comment #5
sunI did not recognize that this is filed against D7.
The quoted lines of D8 are not the effective record of uid1 in D8; the values are just used as a temporary in-memory user session, but not saved to the database.
D8 contains the same placeholders in
user_install()as D7. If we want to change them, then we need to change D8 first.Tagging for backport. That said, I'm not sure whether it is safe to backport this change.
Comment #6
alberto56 CreditAttribution: alberto56 commented@sun, thanks, you are right. The same problem occurs in D8.
Here is D8 grep:
And enclosed is a patch for D8 (the patch at #2 being for D7).
In what way do you think this could be unsafe?
Comment #7
sunLooking at this for a third time, I wonder whether
@localhostwouldn't be more appropriate/failsafe?---
Regarding the backport to D7, an installation profile might rely on these values... very unlikely, but possible. I guess the concern can be ignored, so just pointing it out for transparency.
Comment #8
alberto56 CreditAttribution: alberto56 commentedI was not very comfortable with example.com either. Here is a patch with @localhost.
Comment #9
alberto56 CreditAttribution: alberto56 commentedInterestingly, Drupal does not think admin@localhost is a valid email address, as is documented in #1478656: Unable to use @localhost in email address for site information.
Even though 'placeholder-for-uid-1@localhost' is a sensible placeholder email, perhaps we should use something that validates.
Comment #10
jhedstromI think @localhost is fine here (perhaps it will inspire the fixing of #1478656: Unable to use @localhost in email address for site information).
Regardless, the patch needs a reroll.
Comment #11
jhedstromI think @localhost is fine here (perhaps it will inspire the fixing of #1478656: Unable to use @localhost in email address for site information).
Regardless, the patch needs a reroll.
Comment #12
alberto56 CreditAttribution: alberto56 commentedComment #13
adci_contributor CreditAttribution: adci_contributor commentedTrying to Re-roll
Comment #15
mgiffordRe-uploading for the bots.
Comment #18
dpiIssue is no longer valid.
Simpletest (
\Drupal\simpletest\WebTestBase) and functional web tests (\Drupal\Tests\BrowserTestBase) both get user 1 with username 'admin' and email 'admin@example.com' courtesy of a recent issue #2796105: Move similar methods in BrowserTestBase / WebTestBase to a trait; untangle installDrupal().Kernel (
\Drupal\KernelTests\KernelTestBase) tests dont get any users pre-installed.See
\Drupal\Core\Test\FunctionalTestSetupTrait::initUserSessionfor where the new auth details are defined.Changing to Drupal 7 for consideration