Problem/Motivation

Syslog's config schema currently defines the facility setting as a string, but the corresponding PHP function expects an integer. (And so does the relevant syscall too.)

I found this bug while working on #2500529: Migration path for Syslog 7.x configuration, so the attached patch has been rolled from that.

Proposed Resolution

Change the facility property to an integer.

Remaining Tasks

Review, commit, much rejoicing.

API/UI Changes

None.

Comments

chx’s picture

Issue summary: View changes

Looks good.

chx’s picture

Issue summary: View changes
dawehner’s picture

Status: Active » Needs work

I think we need a hook_update_N as otherwise an import might not work?

jibran’s picture

Status: Needs work » Needs review

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.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.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.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.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.

GeduR’s picture

Hi! I've rerolled the previous patch (the Test path has changed) and also there is a hook update to update the variable from string to integer.

Please review!

Status: Needs review » Needs work

The last submitted patch, 8: 2534760-8.patch, failed testing.

GeduR’s picture

Status: Needs work » Needs review
FileSize
3.13 KB

Attaching a new patch removing the facility default value as the type is now integer and the default value is assigned dynamically on hook_install.

Please review!

dagmar’s picture

Version: 8.3.x-dev » 8.4.x-dev
Status: Needs review » Needs work

Thanks!

+++ b/core/modules/syslog/syslog.install
@@ -13,3 +13,12 @@ function syslog_install() {
+ * syslog.settings.facility should be an integer.

This should say something like. Update syslog.settings.facility from string to integer.

Also, we need an upgrade path test here.

mr.baileys’s picture

Status: Needs work » Needs review
FileSize
1.66 KB
4.33 KB

Added an upgrade path test and updated the hook_update_N comment to match coding guidelines.

dagmar’s picture

Status: Needs review » Needs work

Thanks! Looks pretty good. Some small comments.

  1. +++ b/core/modules/syslog/src/Tests/Update/SyslogUpdateTest.php
    @@ -0,0 +1,39 @@
    +   * @see syslog_update_8300()
    

    This should be syslog_update_8400

  2. +++ b/core/modules/syslog/src/Tests/Update/SyslogUpdateTest.php
    @@ -0,0 +1,39 @@
    +    $config = $this->config('syslog.settings');
    +    $this->assertIdentical('128', $config->get('facility'));
    +
    +    // Run updates.
    +    $this->runUpdates();
    +
    +    $config = $this->config('syslog.settings');
    

    Do we need this $config = ... Twice?

  3. +++ b/core/modules/syslog/syslog.install
    @@ -13,3 +13,12 @@ function syslog_install() {
    +function syslog_update_8300() {
    

    Same here. syslig_update_8400

mr.baileys’s picture

Status: Needs work » Needs review
FileSize
1.14 KB
4.33 KB

Thanks dagmar,

updated the update hook numbering.

Regarding $config: we do need it twice if we want to verify values before and after updates, since otherwise config is not reloaded after running the updates. We could drop the initial assertion though, and just load config and check for the correct value after the updates ran.

dagmar’s picture

Status: Needs review » Reviewed & tested by the community

Thanks @mr.baileys

The last submitted patch, syslog-facility_schema_type-do-not-test.patch, failed testing.

tstoeckler’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/syslog/config/install/syslog.settings.yml
@@ -1,3 +1,2 @@
-facility: ''

I see this was commented above with

Attaching a new patch removing the facility default value as the type is now integer and the default value is assigned dynamically on hook_install.

However, the default configuration should always contain a complete set of all settings, so we should still provide a fallback in this case. It seems 8 (LOG_USER) would be an appropriate fallback here. And since YAML supports comments we could even add a comment like "This will be overridden on non-Windows systems by syslog_install()." or something similar above that line to make this more clear.

In any case we should not remove this setting entirely from the file.

mr.baileys’s picture

Status: Needs work » Needs review
FileSize
547 bytes
4.48 KB

Thanks @tstoeckler, added the default setting for facility back in with the proposed default and comment.

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community

Perfect, thank you!

alexpott’s picture

Updating issue credit - crediting reviews whose comments have resulted in changes to the patch.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 1a4d61e and pushed to 8.4.x. Thanks!

  • alexpott committed 1a4d61e on 8.4.x
    Issue #2534760 by mr.baileys, GeduR, phenaproxima, dagmar, tstoeckler:...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.