| Summary: | ASTERISK-14514: [patch] Gosub() dequotes once more than Macro() | ||
| Reporter: | Ben Winslow (rain) | Labels: | |
| Date Opened: | 2009-07-22 15:33:41 | Date Closed: | 2009-08-06 16:35:07 |
| Priority: | Minor | Regression? | No |
| Status: | Closed/Complete | Components: | Applications/app_stack |
| Versions: | Frequency of Occurrence | ||
| Related Issues: | |||
| Environment: | Attachments: | ( 0) 20090723__issue15557.diff.txt | |
| Description: | Since Gosub started accepting "macro" arguments (before 1.6.0-beta1), it has been dequoting arguments one more time than Macro does. Since Gosub() is meant to supplant Macro() in 1.6 and is already used by AEL to implement macros, this breaks compatibility with 1.4. This behavior also makes it impossible to fetch arguments without quotes being interpreted, which IS possible in a Macro() call through ${ARGn}. It also requires an obnoxious level of quoting if you want quotes to survive all the way to a named macro argument in AEL -- e.g. ¯o(${QUOTE(${QUOTE(${CALLERID(all)})})}); Behind the scenes, Gosub() uses AST_STANDARD_APP_ARGS, which causes ast_app_separate_args() to eat the quotes; Macro() does its own argument parsing. I think the best solution is probably to make Gosub() use non-standard argument parsing as well, but I haven't written a patch yet since I think this warrants more discussion. I verified that this problem exists in 1.6.0-beta1, 1.6.0.10, and 1.6.11. It does not exist in 1.4 because Gosub() doesn't accept extra arguments. ****** STEPS TO REPRODUCE ****** The minimal dialplan in the additional info field illustrates the problem by spitting out the following: Calling macro with versions of '"First Last" <+17005551212>' Quote test - GOSUB - ARG1=First Last <+17005551212> Quote test - GOSUB - ARG2="First Last" <+17005551212> Quote test - GOSUB - ARG3="\"First Last\" <+17005551212>" Quote test - GOSUB - ARG4="\"\\\"First Last\\\" <+17005551212>\"" Quote test - MACRO - ARG1="First Last" <+17005551212> Quote test - MACRO - ARG2="\"First Last\" <+17005551212>" Quote test - MACRO - ARG3="\"\\\"First Last\\\" <+17005551212>\"" Quote test - MACRO - ARG4="\"\\\"\\\\\\\"First Last\\\\\\\" <+17005551212>\\\"\"" ****** ADDITIONAL INFORMATION ****** [debugging] exten => 1234,1,Set(CALLERID(name)=First Last) exten => 1234,n,Set(CALLERID(num)=+17005551212) exten => 1234,n,Verbose(0,Calling macro with versions of '${CALLERID(all)}') exten => 1234,n,Gosub(q0ote_test,s,1(${CALLERID(all)},${QUOTE(${CALLERID(all)})},${QUOTE(${QUOTE(${CALLERID(all)})})},${QUOTE(${QUOTE(${QUOTE(${CALLERID(all)})})})})) exten => 1234,n,Macro(quote_test,${CALLERID(all)},${QUOTE(${CALLERID(all)})},${QUOTE(${QUOTE(${CALLERID(all)})})},${QUOTE(${QUOTE(${QUOTE(${CALLERID(all)})})})}) [quote_test] exten => s,1,Verbose(0,Quote test - GOSUB - ARG1=${ARG1}) exten => s,n,Verbose(0,Quote test - GOSUB - ARG2=${ARG2}) exten => s,n,Verbose(0,Quote test - GOSUB - ARG3=${ARG3}) exten => s,n,Verbose(0,Quote test - GOSUB - ARG4=${ARG4}) exten => s,n,Return() [macro-quote_test] exten => s,1,Verbose(0,Quote test - MACRO - ARG1=${ARG1}) exten => s,n,Verbose(0,Quote test - MACRO - ARG2=${ARG2}) exten => s,n,Verbose(0,Quote test - MACRO - ARG3=${ARG3}) exten => s,n,Verbose(0,Quote test - MACRO - ARG4=${ARG4}) | ||
| Comments: | By: Tilghman Lesher (tilghman) 2009-07-23 10:04:21 Patch uploaded against trunk. By: Ben Winslow (rain) 2009-07-23 14:22:52 I tested this patch against trunk and 1.6.1.1 (with a single, inconsequential reject -- 1.6.1.1 doesn't have LOCAL_PEEK()), and it solves my quoting problems -- thanks! I did accidentally notice, however, that Macro() blindly searches for commas regardless of any quoting, while Gosub() will respect any double quote or backslash quoting that may be in place; so, for example, Macro(foo,a\,b,c) has 3 arguments while Gosub(foo,s,1(a\,b,c)) has 2. Strictly speaking, this is also an incompatibility between the two, but one I hope many fewer people are likely to be bitten by, since blind comma parsing is a nightmare in itself (if 'CALLERID(name)=Last, First', then passing CALLERID(all) as a macro argument is even uglier than the quote problem.) I wanted to point it out, though. By: Tilghman Lesher (tilghman) 2009-07-23 14:58:22 Yes, that's a problem that I wanted to avoid, which is why I altered the standard parsing function, instead of using strsep. By: Ben Winslow (rain) 2009-07-23 15:01:03 I'm happy with the fix as attached. Thanks again. By: Ben Winslow (rain) 2009-08-04 16:16:31 With this patch, macros in AEL pass any ',' in an argument to Set() or MSet() (depending on the compatibility setting) wholesale when setting named argument variables. Because the version of Set is always chosen to be the version compatible with 1.4 behavior, the ',' is interpreted by Set as a request to set multiple variables and causes an error. Would you like me to open a new bug for this, or do you want to handle it in this bug? Example: macro testmacro(arg) { ...anything... } &testmacro("Hello, world!"); Generates (with app_set = 1.6): MSet(LOCAL(arg)=Hello, world!) By: Tilghman Lesher (tilghman) 2009-08-04 17:22:43 I just discussed this with murf, and I believe that always doing a "Set1" is preferable to doing MSet from AEL, but I'm going to let him noodle it for several days to be sure. By: Ben Winslow (rain) 2009-08-05 11:50:34 Argh! I'm terribly sorry, I should've waited until I had a clearer head to submit that bugnote. I dug into this some more today, and it turns out that I accidentally reverted part of your patch while I was making a patch for bug ASTERISK-1479617. I've reapplied it, and now the quoting (" or \) is passed along to (M)Set along with those commas, which prevents any errors. FWIW, I tried the second patch anyway - it works and preserves quotes all the way to the named argument variables (so ${arg} would be '"Hello, world"' in the earlier example), but that behavior was rejected in bug ASTERISK-1126329, so I guess it's all for naught unless you want to reexamine that behavior. I also noticed that the second patch removes \ escapes ('\,' -> ',') when app_set = 1.6, but not when app_set = 1.4 (that's probably moot, though.) I did notice that my example doesn't actually work (AEL doesn't consider quotes at all when separating macro arguments - in my local testing, I was actually using a variable with a comma in its value), but that doesn't really have anything to do with this patch. By: Digium Subversion (svnbot) 2009-08-06 16:29:45 Repository: asterisk Revision: 210908 U trunk/apps/app_stack.c U trunk/include/asterisk/app.h U trunk/main/app.c ------------------------------------------------------------------------ r210908 | tilghman | 2009-08-06 16:29:45 -0500 (Thu, 06 Aug 2009) | 9 lines Allow Gosub to recognize quote delimiters without consuming them. (closes issue ASTERISK-14514) Reported by: rain Patches: 20090723__issue15557.diff.txt uploaded by tilghman (license 14) Tested by: rain Review: https://reviewboard.asterisk.org/r/316/ ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=210908 By: Digium Subversion (svnbot) 2009-08-06 16:32:42 Repository: asterisk Revision: 210909 _U branches/1.6.0/ U branches/1.6.0/apps/app_stack.c U branches/1.6.0/include/asterisk/app.h U branches/1.6.0/main/app.c ------------------------------------------------------------------------ r210909 | tilghman | 2009-08-06 16:32:41 -0500 (Thu, 06 Aug 2009) | 16 lines Merged revisions 210908 via svnmerge from https://origsvn.digium.com/svn/asterisk/trunk ........ r210908 | tilghman | 2009-08-06 16:29:26 -0500 (Thu, 06 Aug 2009) | 9 lines Allow Gosub to recognize quote delimiters without consuming them. (closes issue ASTERISK-14514) Reported by: rain Patches: 20090723__issue15557.diff.txt uploaded by tilghman (license 14) Tested by: rain Review: https://reviewboard.asterisk.org/r/316/ ........ ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=210909 By: Digium Subversion (svnbot) 2009-08-06 16:34:16 Repository: asterisk Revision: 210910 _U branches/1.6.1/ U branches/1.6.1/apps/app_stack.c U branches/1.6.1/include/asterisk/app.h U branches/1.6.1/main/app.c ------------------------------------------------------------------------ r210910 | tilghman | 2009-08-06 16:34:16 -0500 (Thu, 06 Aug 2009) | 16 lines Merged revisions 210908 via svnmerge from https://origsvn.digium.com/svn/asterisk/trunk ........ r210908 | tilghman | 2009-08-06 16:29:26 -0500 (Thu, 06 Aug 2009) | 9 lines Allow Gosub to recognize quote delimiters without consuming them. (closes issue ASTERISK-14514) Reported by: rain Patches: 20090723__issue15557.diff.txt uploaded by tilghman (license 14) Tested by: rain Review: https://reviewboard.asterisk.org/r/316/ ........ ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=210910 By: Digium Subversion (svnbot) 2009-08-06 16:35:07 Repository: asterisk Revision: 210911 _U branches/1.6.2/ U branches/1.6.2/apps/app_stack.c U branches/1.6.2/include/asterisk/app.h U branches/1.6.2/main/app.c ------------------------------------------------------------------------ r210911 | tilghman | 2009-08-06 16:35:07 -0500 (Thu, 06 Aug 2009) | 16 lines Merged revisions 210908 via svnmerge from https://origsvn.digium.com/svn/asterisk/trunk ........ r210908 | tilghman | 2009-08-06 16:29:26 -0500 (Thu, 06 Aug 2009) | 9 lines Allow Gosub to recognize quote delimiters without consuming them. (closes issue ASTERISK-14514) Reported by: rain Patches: 20090723__issue15557.diff.txt uploaded by tilghman (license 14) Tested by: rain Review: https://reviewboard.asterisk.org/r/316/ ........ ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=210911 | ||