syslog.settings.facility should be an integer

Looks good.

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

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!

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!

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.

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

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

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.

Thanks @mr.baileys

+++ 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.

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

Perfect, thank you!

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

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