At http://jaspan.com/dx-files-improving-drupal-developer-experience I propose that we migrate from string literals to defined constants in a variety of contexts: variable_get/set names, permissions, Form API #keys, etc. Attached is a patch that does the first of these: replaces variables names passed to variable_get/set with defined constants.

One complexity is that we have to decide which file should contain the define() for each constant. In this patch, I've used these rules:

- All variables that begin with modulename_ are defined in the corresponding .module file.
- menu_ variables go in menu.inc.
- site_ variables go in common.inc.
- dev_query, which is semi-shared with the devel module, goes in database.inc (will be made obsolete by the new database patch)
- language_ variables go in bootstrap.inc because LANGUAGE_DEFAULT is needed there
- For everything else, the variable is defined in the file which refers to it the largest number of times, with the proviso that any variable which would be defined in .admin.inc or .page.inc is defined in the .module file instead.

I'm certain that there are variables that get misplaced by these heuristics. I've enclosed a summary of all variables and what file this patch defines them in, so please review and make suggestions for what should be moved.

FYI, this entire patch is generated automatically by a perl script, so it is very easy to change the output format, variable to location heuristics/overrides, etc.

Here's the summary:

./includes/actions.inc:
	actions_max_stack

./includes/bootstrap.inc:
	blocked_ips
	cache_inc
	language_
	language_content_type_
	language_content_type_default
	language_content_type_negotiation
	language_count
	language_default
	language_negotiation
	page_cache_fastpath
	reverse_proxy
	reverse_proxy_addresses
	session_inc

./includes/cache.inc:
	cache_flush
	cache_lifetime

./includes/common.inc:
	cache
	cron_last
	cron_semaphore
	css_js_query_string
	date_format_short
	drupal_private_key
	error_level
	file_downloads
	page_compression
	preprocess_css
	preprocess_js
	site_403
	site_404
	site_footer
	site_frontpage
	site_mail
	site_mission
	site_name
	site_offline
	site_offline_message
	site_slogan

./includes/database.inc:
	dev_query

./includes/file.inc:
	allow_insecure_uploads
	file_directory_temp

./includes/image.gd.inc:
	image_jpeg_quality

./includes/mail.inc:
	smtp_library

./includes/menu.inc:
	menu_default_node_menu
	menu_expanded
	menu_masks
	menu_override_parent_selector
	menu_parent_items
	menu_primary_links_source
	menu_primary_menu
	menu_rebuild_needed
	menu_secondary_links_source
	menu_secondary_menu

./includes/password.inc:
	password_count_log2

./includes/session.inc:
	session_write_interval

./includes/theme.maintenance.inc:
	maintenance_theme

./install.php:
	install_locale_batch_components
	install_profile_modules
	install_task
	install_time

./modules/aggregator/aggregator.module:
	aggregator_allowed_html_tags
	aggregator_category_selector
	aggregator_clear
	aggregator_summary_items

./modules/aggregator/aggregator.pages.inc:
	date_format_medium
	feed_default_items
	feed_item_length

./modules/block/block.module:
	block_cache

./modules/blog/blog.pages.inc:
	default_nodes_main

./modules/blogapi/blogapi.module:
	blogapi_node_types

./modules/book/book.module:
	book_allowed_types
	book_block_mode
	book_child_type

./modules/color/color.module:
	color_

./modules/comment/comment.module:
	anonymous
	comment_
	comment_anonymous_
	comment_block_count
	comment_default_mode_
	comment_default_order_
	comment_default_per_page_
	comment_form_location_
	comment_page
	comment_preview_
	comment_subject_field_

./modules/contact/contact.module:
	contact_default_status
	contact_form_information
	contact_hourly_threshold

./modules/dblog/dblog.module:
	dblog_row_limit

./modules/filter/filter.module:
	filter_allowed_protocols
	filter_default_format
	filter_html_
	filter_url_length_

./modules/forum/forum.module:
	forum_block_num_
	forum_block_num_0
	forum_block_num_1
	forum_block_num_active
	forum_block_num_new
	forum_containers
	forum_hot_topic
	forum_nav_vocabulary
	forum_order
	forum_per_page

./modules/locale/locale.module:
	javascript_parsed
	locale_cache_strings
	locale_custom_strings_
	locale_js_directory

./modules/node/node.module:
	node_access_needs_rebuild
	node_admin_theme
	node_cron_comments_scale
	node_cron_last
	node_cron_views_scale
	node_options_
	node_options_book
	node_options_page
	node_preview
	node_rank_
	teaser_length

./modules/profile/profile.module:
	profile_block_author_fields

./modules/search/search.admin.inc:
	overlap_cjk

./modules/search/search.module:
	minimum_word_size
	search_cron_limit
	search_tag_weights

./modules/simpletest/drupal_web_test_case.php:
	install_profile

./modules/simpletest/simpletest.module:
	simpletest_devel
	simpletest_httpauth
	simpletest_httpauth_pass
	simpletest_httpauth_username

./modules/statistics/statistics.module:
	statistics_block_top_all_num
	statistics_block_top_day_num
	statistics_block_top_last_num
	statistics_count_content_views
	statistics_day_timestamp
	statistics_enable_access_log
	statistics_flush_accesslog_timer

./modules/syslog/syslog.module:
	syslog_facility

./modules/system/system.admin.inc:
	clean_url
	date_first_day
	date_format_long_custom
	date_format_medium_custom
	date_format_short_custom
	image_toolkit

./modules/system/system.install:
	cron_key
	cron_threshold_error
	cron_threshold_warning
	file_directory_path

./modules/system/system.module:
	admin_compact_mode
	admin_theme
	configurable_timezones
	date_default_timezone
	date_format_long
	drupal_badge_color
	drupal_badge_size
	drupal_http_request_fails
	system_update_1022
	system_update_6043_RC2
	theme_default
	theme_settings

./modules/taxonomy/taxonomy.module:
	taxonomy_override_selector

./modules/update/update.module:
	update_check_frequency
	update_d6_requirements
	update_fetch_url
	update_last_check
	update_notification_threshold
	update_notify_emails

./modules/upload/upload.module:
	upload_
	upload_extensions_
	upload_extensions_default
	upload_list_default
	upload_max_resolution
	upload_uploadsize_
	upload_uploadsize_default
	upload_usersize_
	upload_usersize_default

./modules/user/user.module:
	password_inc
	user_block_max_list_count
	user_block_seconds_online
	user_block_whois_new_count
	user_email_verification
	user_mail_
	user_mail_status_activated_notify
	user_mail_status_blocked_notify
	user_mail_status_deleted_notify
	user_picture_default
	user_picture_dimensions
	user_picture_file_size
	user_picture_guidelines
	user_picture_path
	user_pictures
	user_register
	user_registration_help
	user_signatures
Files: 

Comments

bjaspan’s picture

Oh, I ran all core tests before and after applying this patch. Results:

# without constants: 4196 passes, 179 fails, 17 exceptions
# with constants: 4194 passes, 181 fails, 17 exceptions

Dries tells me that a second full-test run often has 2 more fails than the first full-test run. I can confirm that the with-constants test did not produce any "undefined constant FOO, assuming 'FOO'" errors (which it did in early versions of the patch until I moved some of the define() statements around).

Actually, I have not run the tests with the most recent version of the patch, I'll do that ASAP.

webchick’s picture

Status: Needs review » Needs work

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.

webchick’s picture

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.

Crell’s picture

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.

bjaspan’s picture

I am completely in favor of #145164 (though I haven't looked at it in a year) and said so in followups to my article. This patch is orthogonal to that one.

I think that the counter-arguments you've presented are not convincing:

- Saying that this patch does not provide PHPdoc for variables is a red herring. We do not currently have PHPdoc for variables. I agree that we should, but that's not a reason to avoid other improvements. I do not see it provided in #145164 either, though clearly it could be.

- Saying that this patch clutters up the global namespace is also a red herring. Currently, we are using a global namespace for variables, namely string literals, that provides no mechanism for detection of a conflict. Using define() will at least detect collisions. If you think we need INSTALL_VARIABLE_INSTALL_PROFILE_MODULES to keep the namespace clean then you must logically think we also need to use 'install_variable_install_profile_modules', which I suspect you do not.

I can see an argument for using (say) V_INSTALL_PROFILE_MODULES to indicate that the define is a variable name. This would eliminate a compile-time conflict with some other use of the constant name. It would also make the code even more readable because it would be more obvious that V_FOO is a variable name than 'foo'. I would certainly support that.

While I do not think either of you have presented a meaningful argument against using defines instead of string literals for variable names, I do think there *are* arguments against it, though they are weak. Namely:

- It requires the programmer to make a separate declaration which is slightly more work. OTOH, #145164 requires slightly more work, too, and yet we are all in favor of it. Good programming discipline is often slightly more typing than sloppy coding but it pays off.

- It requires loading the .module file (or whichever file has the define()) on any request that might touch the variable, which means on any request. #145164 in principle does not since the variable registry can be cached. OTOH, we should not favor poor programming discipline as an "optimization" especially when avoiding loading multiple files is sort of a red-herring issue in itself since any heavily-loaded site can/should be using an opcode cache. Furthermore, if we *really* cared, we could simply declare each module to have an "always load this file if enabled" file (which currently is .module) to define those parts of the module (e.g. API functions not called via a hook) which are always required if the module is enabled.

Again, I totally support the other variable API improvements referenced. Using defined constants will improve those other patches even more.

webchick’s picture

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. :\

Island Usurper’s picture

-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.

bjaspan’s picture

It is certainly seeming like this patch is DOA. I'm not surprised; if the Drupal community did not like using string literals for everything, it wouldn't be. :-) However, I'm going to continue replying to objections I think are invalid so if the patch does not go in it will be clear it was because people "just didn't like it."

Lack of documentation is a red herring because variables are already not currently documented; you seem to be saying "variables need to be documented if they are define()ed but not if they are left as string literals" which seems an illogical position. I certainly agree that all variables should be documented. It is trivial to change my patch so each variable looks like:

/** @variable V_FOO_BAR: TODO add documentation here **/
define('V_FOO_BAR', 'foo_bar');

In fact I've done so; new patch attached (ah, the joys of automatically-generated patches :-).

re: memory, that's an interesting point. First, I observe that all variable name strings total about 3.5k, i.e. "not very much." Furthermore, you are assuming that define()ing a string literal actually consumes more memory than using it as a string literal, which is not at all obvious. I do not know how PHP implements string literals. It may be that every string literal is re-allocated in memory every time it occurs but, since a define() represents a constant, it is allocated only once for all the times it is used. Of course, this also may not be the case, and is probably in the noise margin either way. My point is that you shouldn't assume using define()s consumes more memory.

Regarding dynamically named variables, the patch creates defines for exactly those strings that appear as literals following a variable_get/set call. If there is code that reads

variable_get('node_type_' . $node->type);

then the patch contains

define('V_NODE_TYPE_', 'node_type_');
variable_get(V_NODE_TYPE_. $node->type);

I realize that a variable registry could also detect mis-typed variable names. That would work fine though it will impose a (fairly small) performance penalty.

As for the benefits, I listed them in my original article:

* more readable code
* the ability to change the constant in one place instead of many
* detection of typos at compile time
* auto-completion in IDEs (this is a BIG one for a lot of developers, including walkah apparently)

One more: Many developers whose background includes things other than PHP/Drupal find the plethora of string literals all over Drupal's code to be a major turn-off. We're losing developers and major Drupal-using sites due to what they consider sloppy/non-standard coding practices.

I realize this is not the most important patch/issue ever and I'm not going to lose any sleep over it.

webchick’s picture

Status: Needs work » Needs review

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

bjaspan’s picture

Progress! :-)

Yes, using string literals instead of constants is only one small way of the many that Drupal has poor DX, but fixing it will help, in a small way. This issue is merely the result of my first article in "The DX Files" series. I have several more planned already.

I'll attach the Perl script I wrote to generate the patch (actually, to modify the source tree; cvs diff makes the patch) tomorrow since it is on my work computer. Warning: It's a hack.

gpk’s picture

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.

Crell’s picture

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.

webchick’s picture

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.

webchick’s picture

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'. :)

chx’s picture

Status: Needs review » Closed (won't fix)

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.

chx’s picture

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 :)