Forum OpenACS Development: Re: ad_page_contract optional default will clober a local var value

Jun

Did you change the behavior of an api: yes. Is _that_ a bug: yes. For this you should not even have to ask: don't do it.

Lars is being nice. Unfortunately even if you did post, you might not get any reponse, and you go ahead and make the change. Before you know it we are going to have a toolkit full of junk. And we will end up having to pay folks to use it.

Jun, nothing personal, but we really need to get this habit under control. This is not a place to experiment and see what happens. Many folks have real code that relies on the core apis, and you can't just change the way they work because you want to, for your special situation. This is a very basic principle.

Think about this: we have been using these same procedures for years. What is the chance, after all this time, that what you found is a bug? Maybe at least ask around. And then even if it might be a bug, is it possible someone relies on this behavior?

Example. the ATS 'if' tag has an 'in' operator. To me this is a list, unfortunately it is implemented as a regexp, which gives it some funny behaviors. But in some cases these behaviors are useful, and I have relied on them. But because of the way it works, you can't just pass in a list as a variable. Is that a bug? Yes and no. You can't break the old behavior just to get the thing to work the way you want.

I posted a recommendation to add something like an 'inlist' operator that would work for lists, or lists of lists. No one responded. I assume it is unimportant. I don't put it in. Finished. I still use this on my sites as a convenience to me, but I'm not going to burdon everyone else with my fancy programming abilities.

Example: Lars suggested adding a -unclobber switch to db_multirow. I complained, saying it changed the behavior of the way db_multirow worked. Actually it turns out that db_multirow was changed before, adding the loop found in the db_foreach api. Unfortunately when this was added care was not taken to ensure that db_multirow worked the same way regardless of whether the code block was included or not. When the code block is included, variables get clobbered in the calling environment. When there is no code block this doesn't happen. This is dumb, because you might write a page one day, and everything works. Tomorrow, you add a foreach loop and you have to then go around changing the names of things that get clobbered. It also might not be apparent what is causing the problem, because only the value is incorrect, you don't really get an error with this kind of bug. To me this really was a bug in db_multirow. Lars probably would have been right to completely remove this behavior, but he only suggested a switch to adjust it when needed.

This last example points out something else that is important when making these kinds of changes. Providing background as to the problem you have found helps others evaluate the changes you are suggesting. Not many of us have the time to go look at the code in the detail you have had to to make the change. What you are suggesting could be the perfect solution, but you have to tell us how you got there. There was a reasoning process involved, but the rest of us are starting from scratch.