config.c: Make ast_variable_retrieve return last match. #245
Conversation
ast_variable_retrieve currently returns the first match for a variable, as opposed to the last one. This is problematic because modules that load config settings by explicitly calling ast_variable_retrieve on a variable name (as opposed to iterating through all the directives as specified) will end up taking the first specified value, such as the default value from the template rather than the actual effective value in an individual config section, leading to the wrong config. This fixes this by making ast_variable_retrieve return the last match, or the most recently overridden one, as the effective setting. This is similar to what the -1 index in the AST_CONFIG function does. There is another function, ast_variable_find_last_in_list, that does something similar. However, it's a slightly different API, and it sees virtually no usage in Asterisk. ast_variable_retrieve is what most things use so this is currently the relevant point of breakage. In practice, this is unlikely to cause any breakage, since there would be no logical reason to use an inherited value rather than an explicitly overridden value when loading a config. ASTERISK-30370 #close Resolves: asterisk#244 UpgradeNote: Config variables retrieved explicitly by name now return the most recently overriding value as opposed to the base value (e.g. from a template). This is equivalent to retrieving a config setting using the -1 index to the AST_CONFIG function. The major implication of this is that modules processing configs by explicitly retrieving variables by name will now get the effective value of a variable as overridden in a config rather than the first-set value (from a template), which is consistent with how other modules load config settings.
|
cherry-pick-to: 21 |
github-actions
bot
added
testing-in-progress
test-checks-passed
test-gates-failed
and removed
testing-in-progress
labels
There was a problem hiding this comment.
I'm not comfortable putting this into master at this time. This is a fundamental change in behavior which should go into a standard release first to gather feedback from users. While it is reasonable to believe that it shouldn't impact people, in practice this doesn't always happen.
Makes sense, but I thought breaking changes were always typically master only anyways? If it's going into a standard release already, why wouldn't it also go into master, since that's always the furthest ahead? |
|
Once 21 was branched breaking changes went off the table unless absolutely necessary for some reason. The cut off point is when it is branched, not when the first release candidate occurs. The master branch will next be an LTS. |
Okay, so just to confirm, like some other things at the moment, this will need to wait for ~a year before it can be merged, i.e. it cannot go into either 21 or master at this time? |
|
Correct. |
|
Unless it was done in an optional manner that defaults to maintaining existing behavior. |
Got it. Could I suggest a new tag for issues/PRs, if it's not too much trouble? Something like breaking-change or what not, just so we can easily identify and keep track of things that will need to wait until such a time as breaking changes can be introduced again, e.g. this one and #258 |
I'd rather not add unneeded complexity to work around a bug just to get it merged sooner. I'll wait until next year so a proper fix can go in, that's fine. |
jcolp
added
the
waiting-for-standard-release-development-cycle
|
Label created and added. |
gtjoseph
force-pushed
the
master
branch
7 times, most recently
from
b15287c
to
1862a36
Compare
ast_variable_retrieve currently returns the first match for a variable, as opposed to the last one. This is problematic because modules that load config settings by explicitly calling ast_variable_retrieve on a variable name (as opposed to iterating through all the directives as specified) will end up taking the first specified value, such as the default value from the template rather than the actual effective value in an individual config section, leading to the wrong config.
This fixes this by making ast_variable_retrieve return the last match, or the most recently overridden one, as the effective setting. This is similar to what the -1 index in the AST_CONFIG function does.
There is another function, ast_variable_find_last_in_list, that does something similar. However, it's a slightly different API, and it sees virtually no usage in Asterisk. ast_variable_retrieve is what most things use so this is currently the relevant point of breakage.
In practice, this is unlikely to cause any breakage, since there would be no logical reason to use an inherited value rather than an explicitly overridden value when loading a config.
ASTERISK-30370 #close
Imported from Gerrit: https://gerrit.asterisk.org/c/asterisk/+/19744
Resolves: #244
UpgradeNote: Config variables retrieved explicitly by name now return the most recently overriding value as opposed to the base value (e.g. from a template). This is equivalent to retrieving a config setting using the -1 index to the AST_CONFIG function. The major implication of this is that modules processing configs by explicitly retrieving variables by name will now get the effective value of a variable as overridden in a config rather than the first-set value (from a template), which is consistent with how other modules load config settings.