Steps to reproduce

  • Create an install profile that assigns a value to system.date:timezone.default = Europe/London using config\install\system.date.yml.
  • Run unattended install using drush.
  • Expected: site has timezone Europe/London.
  • Actual: site has timezone UTC (or whatever PHP date_default_timezone_get returns).

Proposal

SiteConfigureForm should check for existing values and use them. The fallback defaults should only be used if there is no existing value.

There are two values that are currently wrong: system.site:site_mail and system.date:timezone.default.

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

AdamPS created an issue. See original summary.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

decafdennis’s picture

I came across this too, working on a patch.

decafdennis’s picture

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.

decafdennis’s picture

Status: Active » Needs review
decafdennis’s picture

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

AdamPS’s picture

Status: Needs review » Needs work

@decafdennis Thanks for the patch.

I can reproduce the site mail issue running the install GUI by hand. The required site mail field appears as blank even though the install profile set a value. This matches reading the code of SiteConfigureForm::buildForm which makes no attempt to read the existing value of site_mail.

$form['site_information']['site_mail'] = [
      '#default_value' => ini_get('sendmail_from'),

I suggest that the patch should have the same logic for site_mail as for the timezone.

I can't explain how your test passes. sendmail_from is a mandatory field. Could it be that in submitForm
$this->config('system.site')->save(TRUE) silently fails, leaving the original correct value in place?

One other minor comment: the line protected function installParameters() { is incorrectly indented.

decafdennis’s picture

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.

decafdennis’s picture

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.

AdamPS’s picture

Status: Needs review » Reviewed & tested by the community

@decafdennis Thanks again, new patch looks good to me.

Good point about drush, however that feels outside the scope of this Drupal core issue. I vote for RTBC.

xjm’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs followup
  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!
decafdennis’s picture

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

The last submitted patch, 13: drupal-profile-site-config-2849074-13-test-only.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

zuuperman’s picture

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

zuuperman’s picture

Status: Needs review » Needs work

The last submitted patch, 15: drupal-profile-site-config-2849074-14-8.4.x.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

decafdennis’s picture

Status: Needs work » Needs review
AdamPS’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me

alexpott’s picture

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.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  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.

decafdennis’s picture

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().

The last submitted patch, 22: drupal-profile-site-config-2849074-22-test-only.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

alexpott’s picture

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.

decafdennis’s picture

Status: Needs review » Needs work
Issue tags: +blocked
alexpott’s picture

Issue tags: -blocked

It's not blocked anymore.

decafdennis’s picture

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.

The last submitted patch, 27: drupal-profile-site-config-2849074-27.test-only.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

sagesolutions’s picture

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.

decafdennis’s picture

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

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.