SiteConfigureForm overrides value from install profile

I came across this too, working on a patch.

Here's a patch that fixes the timezone.default issue as described in the issue summary.

I wasn't able to replicate the same with issue with mail but I did add a test that verifies a site email address provided by the install profile is retained. That means the problem may be with Drush on that one.

I know 8.3.x is done, but here's a patch anyway for anyone who wants it.

Thanks for the feedback! I'll take another look at it and see if I can get a failing test for the site email field.

Here's a new patch that:
* fixes the code indentation issue,
* favors an existing system.site:mail value, if present, over the default ini_get('sendmail_from'),
* tests the default values present on the form, in addition to testing the site config after installer completion.

Note that this still does not fix the unattended installer use case for the site email address field, because Drush has it hardcoded to always default to admin@example.com if --site-mail is not provided.

1. +++ b/core/lib/Drupal/Core/Installer/Form/SiteConfigureForm.php
   @@ -152,10 +152,11 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +    $default_site_mail = $this->config('system.site')->get('mail');
   ...
   -      '#default_value' => ini_get('sendmail_from'),
   

   For readability and maintainability, I think it would be better to put it all in one line for later use in the form array, with a comment:

   // Use the default site mail if one is already configured, or fall back to PHP's configured sendmail_from.
   $default_site_mail = $this->config('system.site')->get('mail') ?: ini_get('sendmail_from');
   

2. We can also do similarly for the date (moving the existing comment about the system timezone above our new line). That way, if we improve this in the future, it's all in one place.
3. Out of scope followup: Sounds like we can remove that @ now since we require PHP 5.5.9? Can we check for an existing issue to remove it or file one if we don't find one?
4. Finally, I don't see a test-only patch attached to this issue to expose the coverage. Let's upload one that is expected to fail, followed by the combined patch that passes. Thanks!

See attached a new patch with requested changes, and a test-only patch.

@xjm: I don't think we can remove that @ as there could still be a warning in PHP 5.5, see https://www.drupal.org/node/2446859

Rerolled patch for 8.4. The default email is already fixed in 8.4. The tests are still new, so I have kept them in the patch

// Edit => Nvm, previous comment also includes a patch for 8.4

The question about the usage of @ and PHP versions was answered in #13 - PHP5.5 is greater than PHP5.4.

I've re-uploaded the patch in #13 as that is the latest and RTBC patch as far as I can tell. The rtbc patch should always be the latest patch since that's how rtbc patches are re-tested.

1. Really nice to see installer tests - it is always great when the installer gets more test coverage.

2. +++ b/core/modules/system/src/Tests/Installer/InstallerSiteConfigProfileTest.php
   @@ -0,0 +1,61 @@
   +  protected $profile = 'testing_site_config';
   +  protected $profile_site_mail = 'profile-testing-site-config@example.com';
   +  protected $profile_timezone;
   

   Class properties should be camelCase and also need a doc block.

   I'm sure though that storing this on the class is really necessary. For the email - just use the hardcoded email in the assert it removes one level of indirection. For timezone maybe a class property is okay. So that would be something like:

     /**
      * The expected timezone.
      */
     protected $expectedTimezone;
   

3. +++ b/core/modules/system/src/Tests/Installer/InstallerSiteConfigProfileTest.php
   @@ -0,0 +1,61 @@
   +  public function __construct($test_id = NULL) {
   

   Rather than doing this in the constructor we should do this in setUp() as this is test set up. It is super rare to override a test constructor.

Thanks for the feedback. Here's a new patch with the following changes:

1. Changed class properties to use camel case and added doc blocks.
2. Changed the class property for the email to a class constant. — @alexpott Does this address your concern? I disagree about the unnecessary level of indirection here, as I would argue that hardcoding it in the asserts would be more of a maintenance issue since we'd be repeating the same constant string value.
3. Moved the initialization of the expectedTimezone class property into setUp(). @alexpott For my understanding, can you say more about why this is necessary? It feels more like object initialization to me, which belongs in the constructor, more than test set up, which would go in setUp().

So with the timezone thing we have an interesting problem!

At the moment the test works because InstallerSiteConfigProfileTest inherits for WebTestBase. All Simpletests will be converted to BrowserTestBase at some point. This means that in the test running process the timezone will be set to 'Australia/Sydney' by bootstrap.php. However because the install is happening in a separate process from test running the install is still occurring with PHP's default timezone. In WebTestBase land the timezone is set to 'Australia/Sydney' in WebTestBase::setUp() but that is still only occurring in the test runner process. Fortunately in regular tests we do a non-interactive install so for the most part all sites are being installed with 'Australia/Sydney' - the exception being any test extending InstallerTestBase. I think we need to open an issue to ensure that sites installed by InstallerTestBase have the expected default timezone of 'Australia/Sydney' and then this test could just use a constant for the timezone and not have to do anything tricky and it won't break when InstallerTestBase gets converted. Opened #2915664: Sites installed by InstallerTestBase should have a timezone of 'Australia/Sydney' to address this. We should postpone this on fixing that.

As re: test class constants for expected values vs just having the string - for me it depends how often it appears. Twice I'd hard code the string.

Blocked by #2915664: Sites installed by InstallerTestBase should have a timezone of 'Australia/Sydney', will need work following.

It's not blocked anymore.

With the timezone in InstallerTestBase being standardized and with other inspiration from #2915664: Sites installed by InstallerTestBase should have a timezone of 'Australia/Sydney' I was able to simplify the patch significantly.

I just tested drupal-profile-site-config-2849074-27.patch and it does keep the timezone as set in the system.date.yml file.

However, the site's email does not get saved from system.site.yml I believe this is due to drush overriding it as per https://drushcommands.com/drush-8x/core/site-install/

--site-mail : From: for system mailings. Defaults to admin@example.com

So when I run drush site-install mycustomprofile the site email is set to admin@example.com instead of whats in the system.site.yml file.

See #10 and #11; Drush is outside the scope of this issue.

Re-rolled for 8.5.x and 8.6.x (they're the same, actually).

We are using the patch from #32 on a Drupal 8.6.x installation and it works ok.

We have tested it using a YAML file on the profile and, also, setting the timezone on the profile's hook_install without any problem.

Looks like this needs a reroll and we should target 8.7.x first. Hopefully the patch will also apply to 8.6.x

Rerolled the patch.

Committed and pushed 04d46f628d to 8.7.x and 55037b509c to 8.6.x. Thanks!

As a non-disruptive bug fix backported to 8.6.x