[Home]

Summary:ASTERISK-10160: "Set" application silently changed in trunk to no longer support multiple assignments
Reporter:Steve Davies . (stevedavies)Labels:
Date Opened:2007-08-24 05:33:07Date Closed:2007-10-02 12:53:39
Priority:MajorRegression?No
Status:Closed/CompleteComponents:Addons/General
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) mset.patch
( 1) mset.patch.2
Description:On Jul 23rd, Tilghman merged the dialplan_aesthetics branch.  (pbx.c went from rev 76555 to rev 76703.

His comment suggests this merge should be transparent except for no longer supporting the App,arg|arg format.

In fact the merge also changes the Set application to no longer support multiple assignments.

So code that looks like so "Set(var1=value1|var2=value2) now suddenly assigns value1|var2=value2 into var1, whereas before it did the two assignments.

Multiple assignments is still sort-of documented in the show application output for Set in trunk.  And it certainly works and is documented in 1.4, and has never been marked or documented as deprecated.

Looks like support for the "g" option is also removed.

Here is the relevant change:

@@ -5867,35 +5871,17 @@
int pbx_builtin_setvar(struct ast_channel *chan, void *data)
{
       char *name, *value, *mydata;
-       int argc;
-       char *argv[24];         /* this will only support a maximum of 24 variables being set in a single operation */
-       int global = 0;
-       int x;

       if (ast_strlen_zero(data)) {
-               ast_log(LOG_WARNING, "Set requires at least one variable name/value pair.\n");
+               ast_log(LOG_WARNING, "Set requires one variable name/value pair.\n");
               return 0;
       }

       mydata = ast_strdupa(data);
-       argc = ast_app_separate_args(mydata, '|', argv, sizeof(argv) / sizeof(argv[0]));
+       name = strsep(&mydata, "=");
+       value = mydata;

-       /* check for a trailing flags argument */
-       if ((argc > 1) && !strchr(argv[argc-1], '=')) {
-               argc--;
-               if (strchr(argv[argc], 'g'))
-                       global = 1;
-       }
-
-       for (x = 0; x < argc; x++) {
-               name = argv[x];
-               if ((value = strchr(name, '='))) {
-                       *value++ = '\0';
-                       pbx_builtin_setvar_helper((global) ? NULL : chan, name, value);
-               } else
-                       ast_log(LOG_WARNING, "Ignoring entry '%s' with no = (and not last 'options' entry)\n", name);
-       }
-
+       pbx_builtin_setvar_helper(chan, name, value);
       return(0);
}


****** ADDITIONAL INFORMATION ******

I rank this major as this creates a user affecting backward compatibility that totally broke my dialplan and will bite others later when everyone has long ago forgotten this.
Comments:By: pj (pj) 2007-08-24 09:55:49

yes, it's right, I also must adjust my dialplan to reflect this change.
By: Tilghman Lesher (tilghman) 2007-08-24 10:02:58

This behavior is clearly deprecated in 1.4.  I'm not sure what else we should do.

Hmm, this is not as clear as it should be.  I'm adding some warnings to 1.4.


By: Digium Subversion (svnbot) 2007-08-24 10:23:40

Repository: asterisk
Revision: 80747

------------------------------------------------------------------------
r80747 | tilghman | 2007-08-24 10:23:39 -0500 (Fri, 24 Aug 2007) | 2 lines

Make the deprecation warning inline with the code, instead of only in documentation (closes issue ASTERISK-10160)

------------------------------------------------------------------------
By: Digium Subversion (svnbot) 2007-08-24 10:31:37

Repository: asterisk
Revision: 80749

------------------------------------------------------------------------
r80749 | tilghman | 2007-08-24 10:31:37 -0500 (Fri, 24 Aug 2007) | 9 lines

Blocked revisions 80747 via svnmerge

........
r80747 | tilghman | 2007-08-24 10:41:43 -0500 (Fri, 24 Aug 2007) | 2 lines

Make the deprecation warning inline with the code, instead of only in documentation (closes issue ASTERISK-10160)

........

------------------------------------------------------------------------
By: Steve Davies . (stevedavies) 2007-08-24 11:17:49

Guys - in the trunk version the "show application set" output _still_ documents multiple arguments.
If I upload a patch to restore the old behaviour, will it be applied or is there some antipathy to the behaviour.

If we are talking about dialplan aesthetics, here are some ways I use it:

exten => s,1,Set(FAX_ACCOUNT=${ARG1},FAX_DID=${ARG2},FAX_EMAIL=${ARG3},LOCALSTATIONID=${ARG4},LOCALHEADERINFO=${ARG5})

exten => _1.,1,Set(ARG1=${CUT(EXTEN,`,2)},ARG2=${CUT(EXTEN,`,3)},ARG3=${CUT(EXTEN,`,4)},ARG4=${CUT(EXTEN,`,5)})
exten => _1.,n,Set(CDR(accountcode)=${ARG1},ACCOUNTCODE=${ARG1},FROM-PEER=${ARG2})

[macro-rewrite-callerid]
exten => s,1,GotoIf($[x${ORIG_CID} != x]?3)  ; first time ever in here we remember the originally sent cid
exten => s,2,Set(ORIG_CID=${CALLERID(num)})
exten => s,3,Set(stop=N|argc=0|CALLERID(num)=${ORIG_CID})
exten => s,4,GotoIf($[x${stop} = xY]?99)
exten => s,5,Set(argc=$[${argc} + 1])
exten => s,6,GotoIf($[x${ARG${argc}} = x]?99)
exten => s,7,Macro(one-rewrite,${ARG${argc}})
exten => s,8,Goto(4)
exten => s,99,Verbose(3,Rewriter started with ${CID_PLAN}${ORIG_CID} and ended with ${CALLERID(num)})

Steve
By: Digium Subversion (svnbot) 2007-08-24 14:32:12

Repository: asterisk
Revision: 80817

------------------------------------------------------------------------
r80817 | tilghman | 2007-08-24 14:32:11 -0500 (Fri, 24 Aug 2007) | 2 lines

Fix documentation for Set (closes issue ASTERISK-10160)

------------------------------------------------------------------------
By: Digium Subversion (svnbot) 2007-08-24 16:59:54

Repository: asterisk
Revision: 80877

------------------------------------------------------------------------
r80877 | murf | 2007-08-24 16:59:52 -0500 (Fri, 24 Aug 2007) | 70 lines

Merged revisions 80790,80817,80819,80821,80850 via svnmerge from
https://origsvn.digium.com/svn/asterisk/trunk

................
r80790 | murf | 2007-08-24 13:03:39 -0600 (Fri, 24 Aug 2007) | 9 lines

Merged revisions 80789 via svnmerge from
https://origsvn.digium.com/svn/asterisk/branches/1.4

........
r80789 | murf | 2007-08-24 12:52:15 -0600 (Fri, 24 Aug 2007) | 1 line

From a complaint by jmls, I realize that the message in cdr_disposition is unnecessary. To get failure disposition, just return -1; no use having more than one case do that.
........

................
r80817 | tilghman | 2007-08-24 13:50:16 -0600 (Fri, 24 Aug 2007) | 2 lines

Fix documentation for Set (closes issue ASTERISK-10160)

................
r80819 | bweschke | 2007-08-24 14:21:17 -0600 (Fri, 24 Aug 2007) | 11 lines

Merged revisions 80818 via svnmerge from
https://origsvn.digium.com/svn/asterisk/branches/1.4

........
r80818 | bweschke | 2007-08-24 15:52:06 -0400 (Fri, 24 Aug 2007) | 3 lines

A minor correction to the available logic of autofill. If a queue member is paused, they're not really "available" so don't count them as such. Somewhat related to issue ASTERISK-9835


........

................
r80821 | russell | 2007-08-24 14:25:39 -0600 (Fri, 24 Aug 2007) | 15 lines

Merged revisions 80820 via svnmerge from
https://origsvn.digium.com/svn/asterisk/branches/1.4

........
r80820 | russell | 2007-08-24 15:24:05 -0500 (Fri, 24 Aug 2007) | 7 lines

Improve the debouncing logic in the DTMF detector to fix some reliability
issues.  Previously, this code used a shift register of hits and non-hits.
However, if the start of the digit isn't clean, it is possible for the
leading edge detector to miss the digit.  These changes replace the flawed
shift register logic and also does the debouncing on the trailing edge as well.
(closes issue ASTERISK-10148, many thanks to softins for the patch)

........

................
r80850 | russell | 2007-08-24 15:23:14 -0600 (Fri, 24 Aug 2007) | 13 lines

Merged revisions 80849 via svnmerge from
https://origsvn.digium.com/svn/asterisk/branches/1.4

........
r80849 | russell | 2007-08-24 16:22:50 -0500 (Fri, 24 Aug 2007) | 5 lines

If dnsmgr is in use, and no DNS servers are available when Asterisk first
starts, then don't give up on poking peers.  Allow the poke to get rescheduled
so that it will work once the dnsmgr is able to resolve the host.
(closes issue ASTERISK-10138, patch by jamesgolovich)

........

................

------------------------------------------------------------------------
By: Tilghman Lesher (tilghman) 2007-08-26 01:43:48

stevedavies:  We're specifically trying to make the Set application behave correctly when you retrieve a value from a database that contains a comma.  With Set taking multiple arguments, it behaves incorrectly.  Having users separate their Set name/value pairs into separate lines is not a big change, while making database integration easier.  That's why we've chosen to go down this path.

I realize that this causes an issue with some dialplans, including some of my own, but I think this also is the right way to go to allow us to create even more complex dialplans in the future, without making the syntax difficult to read.
By: Steve Davies . (stevedavies) 2007-08-26 01:51:25

> stevedavies: We're specifically trying to make the Set application behave correctly when you retrieve a value from a database that contains a comma.

Thanks for the explanation.  But try to excuse the grumble:  Set was changed some time ago (early 1.2 cycle maybe?) to support multiple assignments.  Users are taking advantage of the feature.  Now there's a side-effect impact that wasn't considered at the time.  So now a year or more later it is silently removed again.

Maybe I'm blind, but where do I find that multiple-assignment in Sets has been deprecated in 1.4?

Steve
By: Steve Davies . (stevedavies) 2007-08-29 16:03:46

I've uploaded a patch that's an attempt at a compromise.

Set() remains with the changed behaviour of only allowing a single var=value.

But I've resurrected the old pbx_builtin_setvar and made it available as "MSet".

People will still have the change their dialplan but at least the old semantics are still available via MSet.

I feel this is necessary because I tried to update my dialplan (which is spread across 100+ files) and uses some tricky suff with exten priorities etc and it was far far far from simple to do so.  

This was I can just change Set to MSet where I know I'm intending to do multiple assignments.

Best of both worlds - no nasty suprises if a value unexpectedly contains a comma, but no loss of functionality.

Stev
By: Steve Davies . (stevedavies) 2007-08-29 16:30:01

hmm - mset doesn't work.  stand by for an updated patch
By: Steve Davies . (stevedavies) 2007-08-29 16:36:53

mset.patch.2 has a version of the patch that works properly
(nearly the equivalent of Set as was, except that the obsolete | argument delimiter isn't supported)
By: David Chappell (chappell) 2007-09-28 12:28:53

Wouldn't it be better to leave Set() as it was (accepting multiple values) and introduce a new function for setting values which may contain commas?  Switching the behavior of Set() will cause lots of subtle bugs in dialplans which work now.

I believe we really need a way to set multiple values at one priority.  I was able to greatly simplify my dialplan and reduce the potential for silly errors by merging all blocks of calls to Set() into a single call which set multiple values at once.  This is particularly helpful when a phone number must be recognized and several variables set, such as the caller ID name and channel language.
By: Tilghman Lesher (tilghman) 2007-09-28 12:45:25

chappell:  You still can, even with this change, by using the ARRAY dialplan function:

Set(ARRAY(one,two,three)=1,2,3)
By: Tilghman Lesher (tilghman) 2007-10-01 10:26:38

stevedavies:  in fact, I'm tempted to say "Use the ARRAY function", instead of adding MSet.  We're trying to get away from bad syntax, not create alternative ways for you to have the old, poor syntax.
By: Digium Subversion (svnbot) 2007-10-02 12:53:39

Repository: asterisk
Revision: 84405

U   trunk/main/pbx.c

------------------------------------------------------------------------
r84405 | tilghman | 2007-10-02 12:53:38 -0500 (Tue, 02 Oct 2007) | 2 lines

Add MSet for people who prefer the old, deprecated syntax of Set (Closes issue ASTERISK-10160)

------------------------------------------------------------------------