Replace variable string literals with defined constants

None of these are PHPDoced.

/**
 * Variables
 */
define(...)
define(...)
define(...)

is not correct.

It needs to be:

/** Description of this variable. **/
define(...)

/** Description of this variable. **/
define(...)

I've read the referenced article. I'm not sure I agree that this makes things easier. Seeing a reference to INSTALL_PROFILE_MODULES doesn't tell me that INSTALL_PROFILE_MODULES is a variable. We'd need some careful name-spacing like INSTALL_VARIABLE_INSTALL_PROFILE_MODULES or something like that, which ends up being *more* code for me as a developer to type in, which results in poor DX from my perspective.

In terms of centralizing variable names and defaults (which I agree is a good goal), I like the approach being taken at http://drupal.org/node/145164. That way, variable get calls become:

$default = variable_get('theme_default');

rather than

$default = variable_get('theme_default', 'garland');

and we get a nice centralized place within each module to see the list of variables it exposes and what their default values ought to be. It also helps out with i18n of user-facing string variables, which is another plus.

Oh, the other reason I don't like defines is that they crud up the global namespace. There's little point in USER_PICTURE_DIMENSIONS being loaded on every single page load when I only care about its setting when I'm actively printing out or validating a user picture, and even then only when I have user pictures set to on.

Honestly, I'm -1 on this. I'd much rather see #145164: DX: Use hook_variable_info to declare variables and defaults go in, which has a number of benefits in addition to being able to catch spelling mistakes by whining loudly on an undefined variable key. #79008: make variable prefetching optional is also related.

Huh? The documentation complaint is not a red herring. All of Drupal's define()s /do/ have documentation (or should. they did at one point, because I spent like 3 weeks tracking them all down, but some might have snuck in since then, or some of my patches might not have been committed.) So if we're going to convert them to define()s, they need to be PHPDoced. At the very least, you should remove the incorrect PHPDoc above a group of variables, which will incorrectly only document the first one in the list as "Variables." and let someone else go and clean up after (sigh...)

The second reason I think was misunderstood, probably because I misspoke. By "global namespace" I mean "loading the values of those things into memory." Your output there shows 100-some defines that would be loaded up on every page load, and that's in core alone. On any given page, I probably *actually* need like 6 of those. With all the rather extreme movements in D7 lately to reduce Drupal's memory footprint, this seems a curious move. The point you bring up about .module files having to be forced to be loaded also works against the registry patch that just went in after 2+ months of work.

Another reason which I previously didn't mention, but which occurs to me now is how dynamic variable names are handled, like 'node_options_book' or whatever. I'm actually not sure how the variable registry patch handles that either. But I notice you're doing:

+define('NODE_OPTIONS_BOOK', 'node_options_book');
+define('NODE_OPTIONS_PAGE', 'node_options_page');

What about ARTICLE and what about RANDOM_NODE_TYPE_I_JUST_CREATED? That might be why the tests are failing, not sure.

The main point of this patch seems to be "PHP will warn me if I misspell a variable name" (which is true only if notices are being displayed), but we could easily work such a mechanism into #145164 that could work under all conditions.

I'm missing the big win here, I guess. :\

-1

I actually find defined constants harder to debug than string literals. This is because they are output in error messages as the value rather than the label. It's another speed bump in figuring out where the data comes from. It's not a big one since it's the same words, but I have to realize I should be looking for a defined constant first.

Also, ALL CAPS are harder to read than lower case, but I'm aware that's not a very strong argument in and of itself.

Thanks for addressing my concerns, even though I know you don't agree with them. I like this second iteration much better, both from the variable name-spacing standpoint and from the fact that it won't leave hundreds of gaping holes @ http://api.drupal.org/api/constants. I'd prefer VAR_XXXX but that's minor. ;)

The memory I will confess is likely a true red herring. Although it'd be interesting from an academic point of view to do a before/after RAM consumption comparison on a 'typical' Drupal site that has 60+ contributed modules on it. Can you attach the script you're using to generate the patch?

But as to this list: (this is fun ;))
* more readable code

That's subjective. I personally find 'site_name' much more readable than V_SITE_NAME.

* the ability to change the constant in one place instead of many

The variable registry gives us that too.

* detection of typos at compile time

Sure, this is a benefit.

* auto-completion in IDEs (this is a BIG one for a lot of developers, including walkah apparently)

I don't use an IDE, but sure, this is a benefit.

But this...

We're losing developers and major Drupal-using sites due to what they consider sloppy/non-standard coding practices.

That statement I absolutely agree with. But those type of people are much more likely to leave Drupal for any of the following reasons:

* our external-facing APIs are a complete and utter mess (node_delete() vs. taxonomy_del_vocabulary(), etc. .. and variable_get('variable', 'DEFAULT_VALUE') for that matter. :P~~~~~)
* we don't embrace OOP, and in fact have many people who actively work against moving any code to objects
* our testing framework is a massive train-wreck, at least atm (I think about 5 of our existing tests pass with no issues, down from 15 or so when the testing framework was initially committed)

...than they are because we use literal strings instead of constants. Come on now. :P

OK speaking as someone with probably rather less development experience than any of you folks, while I'd agree that there are many string literals that could usefully be replaced by constants, I don't think that the names of static variables fall into that category. It seems to be introducing an unnatural additional layer of redirection. I mean, we aren't going to start doing ${USER} are we ?! Also, I know that I will easily find variable 'site_name' in the {variables} table. But it just feels odd if generally we have V_VARIABLE_NAME corresponding to 'variable_name' in {variables} but on the odd occasion where a variable has been renamed we have V_VARIABLE_NAME corresponding to 'another_variable' in {variables}.

Sorry not to be more supportive of a genuine attempt to improve the code!

But as for other string literals then yes, absolutely.

I'm all for improving DX, but I do not think this is a good way to do it. As gpk points out, it's another layer of indirection in which things can get relabeled and broken. It's like a CSS class of ".blue".

A registered variables info hook (link above) would provide the same end goal -- the ability to throw loud warnings on typos in variable names during development and thereby prevent hard to track bugs -- as well as a host of other benefits and not have the "class blue" problem.

agreed. solving the problem of having to re-input the default value of a variable every time i get it and having to find/replace that default value in all my module's files when I decide to set it to something different (or, worse, all of the contrib modules using my variable, which I don't have control over :\), is a far more severe DX problem than remembering the name of the variable, which rarely changes. And the other patch is a more robust fix for these problems.

Also, I don't think this is at all clear from my previous replies, but I hugely applaud your efforts to improve Drupal's DX and I think this is an extremely worthy goal. I just want to make sure that people who share this vision are focusing efforts on patches that give us the biggest 'bang for our buck'. :)

I am sorry but this is not good developer experience. Constants are needed if they are much more expressive than a value, for example define('DRUPAL_BOOTSTRAP_SESSION', 4); it's easier to write DRUPAL_BOOTSTRAP_SESSION than remembering that it is four. Also, this is something other scripts and parts of code might want to use. I do not see how a constant instead the string page_compression is more expressive nor can I think of any sensible use case where you would want to reuse this variable.

Oh one more thing: it's a _fantastic_ idea to improve DX but more often than not there is no 'one size fits all' megapatch that can lead to that. Small moves, Barry, small moves :)