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.
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.
Comment | File | Size | Author |
---|---|---|---|
#18 | 2534760-18.patch | 4.48 KB | mr.baileys |
#18 | interdiff.txt | 547 bytes | mr.baileys |
#14 | 2534760-14.patch | 4.33 KB | mr.baileys |
#14 | interdiff.txt | 1.14 KB | mr.baileys |
#12 | 2534760-12.patch | 4.33 KB | mr.baileys |
Comments
Comment #1
chx CreditAttribution: chx commentedLooks good.
Comment #2
chx CreditAttribution: chx commentedComment #3
dawehnerI think we need a hook_update_N as otherwise an import might not work?
Comment #4
jibranComment #8
GeduR CreditAttribution: GeduR at Metadrop commentedHi! 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!
Comment #10
GeduR CreditAttribution: GeduR at Metadrop commentedAttaching 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!
Comment #11
dagmarThanks!
This should say something like. Update syslog.settings.facility from string to integer.
Also, we need an upgrade path test here.
Comment #12
mr.baileysAdded an upgrade path test and updated the hook_update_N comment to match coding guidelines.
Comment #13
dagmarThanks! Looks pretty good. Some small comments.
This should be syslog_update_8400
Do we need this
$config = ...
Twice?Same here. syslig_update_8400
Comment #14
mr.baileysThanks 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.
Comment #15
dagmarThanks @mr.baileys
Comment #17
tstoecklerI see this was commented above with
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.
Comment #18
mr.baileysThanks @tstoeckler, added the default setting for facility back in with the proposed default and comment.
Comment #19
tstoecklerPerfect, thank you!
Comment #20
alexpottUpdating issue credit - crediting reviews whose comments have resulted in changes to the patch.
Comment #21
alexpottCommitted 1a4d61e and pushed to 8.4.x. Thanks!