Parameter names should have underscores separating words. For one thing, they are easier to read, especially for those whose native language isn't English. For another thing, underscores can be converted into spaces when parameter names are displayed (f.e. in editparams.cgi) making them even more user-friendly.

Hmm, is this still worth doing in light of the editparams rewrite which is shortly going to eliminate the user-visibility of param names? I suppose it would still make things easier for developers who have to deal with them on the back end though...

I am taking this bug, and plan to work on it to completion within the next 30 days. This is a far-reaching bug. It is probably going to end up touching at least 80% of the files in Bugzilla, since almost every one of them uses parameters *somewhere*. As such, before I embarked on actual work, I thought I'd solicit comments from people -- especially those doom-and-gloom, "Stop, you can't do this because..." or "I'm gonna r- any patches on this bug into infinity because..." sorts of comments. (I'd also love helpful, design-oriented comments, but I'm more worried about someone trying to stomp me flat, which is why I'm trying to uncover such objections ahead of time.) I don't think that anyone is going to disagree 'in principle'... but principle and practice are not always the same thing. :) I've added everyone I can think of who may have something to say to the cc: list; add more/tell more folks if you think they'll care. Any oversight was purely unintentional. My plan is as follows: 1) Wherever they appear (in code or in documentation), I will be renaming the following parameters. (Please say if you disagree with any of the places where I've broken for word boundaries.) LDAPBaseDN to LDAP_BaseDN LDAPbinddn to LDAP_binddn LDAPfilter to LDAP_filter LDAPmailattribute to LDAP_mail_attribute LDAPserver to LDAP_server LDAPuidattribute to LDAP_uid_attribute allowbugdeletion to allow_bug_deletion allowemailchange to allow_email_change allowuserdeletion to allow_user_deletion browserbugmessage to browser_bug_message chartgroup to chart_group commentonaccept to comment_on_accept commentonclearresolution to comment_on_clear_resolution commentonclose to comment_on_close commentonconfirm to comment_on_confirm commentoncreate to comment_on_create commentonduplicate to comment_on_duplicate commentonreassign to comment_on_reassign commentonreassignbycomponent to comment_on_reassign_by_component commentonreopen to comment_on_reopen commentonresolve to comment_on_resolve commentonverify to comment_on_verify confirmuniqueusermatch to confirm_unique_user_match cookiepath to cookie_path createemailregexp to create_email_regexp cvsroot to cvsroot cvsroot_get to cvsroot_get defaultlanguage to default_language defaultpriority to default_priority defaultquery to default_query defaultseverity to default_severity emailregexp to email_regexp emailregexpdesc to email_regexp_desc emailsuffix to email_suffix enablequips to enable_quips insidergroup to insider_group letsubmitterchoosepriority to let_submitter_choose_priority loginnetmask to login_netmask makeproductgroups to make_product_groups maxattachmentsize to max_attachment_size maxpatchsize to max_patch_size maxusermatches to max_user_matches mostfreqthreshold to mostfreqthreshold move-button-text to move_button_text move-enabled to move_enabled move-to-address to move_to_address move-to-url to move_to_url moved-default-component to moved_default_component moved-default-product to moved_default_product moved-from-address to moved_from_address musthavemilestoneonaccept to must_have_milestone_on_accept mybugstemplate to mybugs_template newchangedmail to newchanged_mail noresolveonopenblockers to no_resolve_on_open_blockers passwordmail to password_mail rememberlogin to remember_login requirelogin to require_login sendmailnow to send_mail_now shadowdb to shadow_db shadowdbhost to shadow_db_host shadowdbport to shadow_db_port shadowdbsock to shadow_db_sock showallproducts to show_all_products shutdownhtml to shutdown_html supportwatchers to support_watchers timetrackinggroup to time_tracking_group timezone to time_zone urlbase to url_base usebugaliases to use_bug_aliases useclassification to use_classification useentrygroupdefault to use_entry_group_default usemenuforusers to use_menu_for_users useqacontact to use_qa_contact usermatchmode to user_match_mode usestatuswhiteboard to use_status_whiteboard usetargetmilestone to use_target_milestone usevisibilitygroups to use_visibility_groups usevotes to use_votes voteremovedmail to vote_removed_mail webdotbase to webdot_base whinedays to whine_days whinemail to whine_mail 2) Where there are identically-named local variables, I will rename them too. So: my $urlbase = Param("urlbase"); will become my $url_base = Param("url_base"); (and not: my $urlbase = Param("url_base"); I would like to hear pros and cons on this decision. IMHO, there are two schools of thought: a) Touch as little as possible: make the minimal required changes necessary to present this change to the user, or b) Be as thorough as possible: make all the changes necessary everywhere. Method a) would be easier on heavily-modified codebases, as it is likely that the parameter is only ever read once, and then stored in a local variable that is repeatedly referenced. If we leave that local variable alone, then it'll be less work for me, and easier for others to apply the patch. successfully. The second method is better programming practice. It's a lot more work (for me, and potentially for somone applying the patch), but it does the job *right*. The whole point of this bug is to clean up the code and make it more 'professional'. It's a lot of tedious, picky work... but I don't mind doing that work, or I wouldn't have taken it... and if I'm going to do it at all, I'd like to do it _properly_. That way it doesn't have to be done again in the future, and also we won't end up explaining to future contributors why it's okay for legacy code to say "my $paramname = Param(param_name)" but they're not allowed to these days. So, those are my thoughts. Anyone else?

Before anyone notices and comments, this was a mistake: mostfreqthreshold to mostfreqthreshold and should have been: mostfreqthreshold to most_freq_threshold This was *not* a mistake: cvsroot to cvsroot because that's the name of the actual ENV parameter.

Um, I hate to be the doom-and-gloom-sayer, but changing variable names just to make them easier to read has these pluses and minuses: Plus: The code becomes easier to read. Minus: Any custom code that uses these variables breaks. I don't think that really works out to "we should do this, it's important." :-) I would actually WONTFIX this, particularly if the editparams rewrite is taking away param names from the user.

A comment on proposal b) - there's no reason why you couldn't convert the parameters a few at a time, in a series of smaller patches, in other to make review much easier. It would also avoid the problem of an uber-patch rotting quickly. Gerv

Having given this some thought, it looks like params rewrite (bug 46296) is going to remove the immediate user-wise effect the param names have. However, it is also true that many of the params have a significant effect in the code. Striving to clean that up is a worthy aim, and one that cannot (or even should not) be easily done with the other patches. However, changing the names is a considerable headache for customizers. We should start this work at the beginning of a new version cycle (after 2.20 has branched) and try to finish it prior to 2.22 going to rc. That said, a single patch would be totally hellish. Also, the changes we make here should also be simultaneously used for cleaning up the semantics of the parameter names - while some of the issues below may sound cosmetic, I don't think we're going to have as good a chance for fixing up these cosmetics as we're going to have now, if we start adding underscores anyway. About the variable names: There are multiple inconsistencies in the way the params are named. I believe we should work towards unifying some of the param naming concepts (and perhaps documenting them in the dev guidelines as well) while we're at this. I'm referring to following kinds of issues: - param names should be in lowercase (LDAP_foo are the only exception) - the feature-enabling booleans could be named more uniformly. Although there is a semantic difference between "allow..." and "use...", in practice these options often come very close to one another. Perhaps it would be cleaner to have a simple naming scheme for these; I suggest "enable_xxx". Things like support_watchers and musthavemilestoneonaceppt also belong to this group. - The "require"-params (such as commentonaccept, noresolveonopenblockers and stuff) should have the "require_" prefix to make it clear what they actually mean. - Since we're not going to have the pref names user-visible for long, we could just as well make them verbose enough to be clear in the code. Stuff like "makeproductgroups" should be expanded to "automatically_create_product_bug_groups" or whatever (that example might've exaggerated things a bit, but I believe the idea gets through: Some of the params have such esoteric names that even the developers can't remember their semantics. Although names shouldn't contain documentation, they should contain a really good indication of the meaning. Another example of the need is "usemenuforusers"... eww. - Some prefixing to group the properties together would be preferable. For example, the authentication related params could be grouped under the "auth_" prefix. We already have this for some of the params, but LDAP stuff could be prefixed as well. Another candidate would be mail related params (mail_ prefix), even though many of them (the template related ones) are going away anyway. All that said, I feel it would be appropriate to first give some thought to the generic param naming guidelines, then make this a meta bug and start renaming the params in some reasonably sized groups, each of this work progressing in a separate bug. That way, if some discussion on a certain param's new name rises, it will be contained in a reasonably small bug (instead of this becoming a 200+ comment monster á la bug 84876). Thoughts?

I should point out that I hadn't read the discussion in developers@ before posting the previous comment (silly me, but I happened to read bugmail first). The desire to have an aliasing layer to avoid customization breakage is a sane one, and I believe it should be done. This applies whether we just do underscore additions or semantic renames. Now I'll go post my previous comment in developers@ too.

What about adding "DisplayName" for each parameter? No need for parameter name changes, but this would allow proper localization=

Well, I think that Jouni's comments on this bug show a tremendous amount of logic and insight. I completely agree that if this going to be done, it would be a good time to regularize the parameters from the past and provide guidelines for the future. Prefixing, generic param renaming guidelines, starting with a release cycle and attempting to finish before the next freeze... all good ideas. It's probably correct that this would be a hellish patch to do all at once, so it's a good idea to have the discussion here on which ones need changing and how they can be properly organized before actually tackling anything. As such, this can definitely become a meta-bug, and we can set dependencies appropriately as it is decided which chunk to split off next. BUT... I don't want to start doing that until/unless I hear back from Dave that he gives his blessing to this being done. In IRC, I was told that we would wait for the discussion to die down, then he would consider it and give a go/no go on the whole issue. Well, discussion seems to have pretty much died down... so which is it, Dave? Do we continue discussion with the explicit pre-approval of the intent (while still requiring explicit approval on each explicit patch), or do we stop here?

Since the intention on the back end is to break defparams.pl into a series of .pm files that each cover a specific functional area, perhaps making the name something that easily allows to you tell which .pm file it's defined in would also be helpful... I'm very much in favor of this once we branch, by the way (or sooner if we call off the 2.20 freeze). We do need to write it up for the developer guide.

Dave: grep is your friend :-) We don't want to have to rename parameters if we ever move them from one category to another for some reason. Gerv

My vote for WONTFIX! This is going to break all extensions and customizations using parameters, and I see absolutely no benefit in renaming these parameters. The only ones seeing these names are admins, nobody else.

I think it would be nice, and I think we should have a standard ("use lower case and separate with underscores") for newly-created parameters, but I don't think it's ever going to be worth it to go back through and rename all the existing ones with a compatibility layer. Gerv