Add constants to magic numbers

ooh - nice. code looks good. i hope to test this.

Do we want to reconsider this again? Issue for 5.x that was marked won't-fix, http://drupal.org/node/43355

-1 as I tend to agree with the conclusions from the 5.x issue. For simple boolean values, 0 and 1 are good enough. If we had a negative logic situation (i.e., status = 0 means published), I could see doing this, but that's not the case for these three.

I vote +1 for self documenting code vs magic numbers. Magic numbers are very dangerous!

The Wikipedia page, http://en.wikipedia.org/wiki/Magic_number_(programming)#Unnamed_numerical_constant , provides some background.

In this case how much code would I have to read to find meaning of status=0 is published? I know that PHP is interpreted, so the overhead of looking up the constants is a issue. But, good software engineering should be the rule.

i think this is desireable for consistency. that issue was not marked won't fix by a committer so it really doesn't represent a "decision". the documentation here is a nice bonus.

I'm also +1 on constants over magic numbers, even when there's just 2 of them. The performance hit is so small as to not be worth mentioning. Also, it's all in compile time. If you're running an Opcode Cache, there isn't even a cycle's worth of cost.

A nice idea - but I have to think the patch is incomplete?

Also, if we're going down this road, there are some even more serious magic number issues in menu.inc that deserve this treatment first.

+1 for this patch. Very handy for more than 2 choices and consistent with comments. Also: status = 1 means published ? not so clear as promote = 1.
+1 to look for other magic number issues but this doesn't mean this patch is incomplete. I tested it and, for me, it's RTBC

Well, pwolanin's point is that lots of places, the magic numbers are still used for status, promote, etc.

He is talking about the menu.inc and I only could find some

$cache_cleared[$menu_name] == 1

and

$cache_cleared[$menu_name] = 2

but nothing else.

I did a grep also looking for '$node->status = 1' and '$node->promote = 1' and only could find the ones already patched here....so I don't know what are those magic numbers actually pwolanin is referencing to.

Let me give some examples. Not all of them are node status, but well... We can also look for INSERTs, where there is no such clear pattern to look for, as well as array formats for submitted forms, etc. In short I am not convinced that having a small number of these magic numbers disappear from core is what we should concentrate this close to a release. There are lots of places to introduce this if you aim for consistency.

$ grep -r "status = 1" *
includes/locale.inc:  $query = "SELECT name, filename FROM {system} WHERE status = 1";
includes/locale.inc:    $result = db_query("SELECT name, filename FROM {system} WHERE status = 1");
includes/module.inc:        $result = db_query("SELECT name, filename, throttle FROM {system} WHERE type = 'module' AND status = 1 AND bootstrap = 1 ORDER BY weight ASC, filename ASC");
includes/module.inc:        $result = db_query("SELECT name, filename, throttle FROM {system} WHERE type = 'module' AND status = 1 ORDER BY weight ASC, filename ASC");
modules/block/block.module:        $result = db_query("SELECT DISTINCT b.* FROM {blocks} b LEFT JOIN {blocks_roles} r ON b.module = r.module AND b.delta = r.delta WHERE b.status = 1 AND b.custom != 0 AND (r.rid IN (". db_placeholders($rids) .") OR r.rid IS NULL) ORDER BY b.weight, b.module", $rids);
modules/block/block.module:    $result = db_query(db_rewrite_sql("SELECT DISTINCT b.* FROM {blocks} b LEFT JOIN {blocks_roles} r ON b.module = r.module AND b.delta = r.delta WHERE b.theme = '%s' AND b.status = 1 AND (r.rid IN (". db_placeholders($rids) .") OR r.rid IS NULL) ORDER BY b.region, b.weight, b.module", 'b', 'bid'), array_merge(array($theme_key), $rids));
modules/blog/blog.module:      $result = db_query_range(db_rewrite_sql("SELECT n.nid, n.title, n.created FROM {node} n WHERE n.type = 'blog' AND n.status = 1 ORDER BY n.created DESC"), 0, 10);
modules/blog/blog.pages.inc:  $result = pager_query(db_rewrite_sql("SELECT n.nid, n.sticky, n.created FROM {node} n WHERE n.type = 'blog' AND n.uid = %d AND n.status = 1 ORDER BY n.sticky DESC, n.created DESC"), variable_get('default_nodes_main', 10), 0, NULL, $account->uid);
modules/blog/blog.pages.inc:  $result = pager_query(db_rewrite_sql("SELECT n.nid, n.created FROM {node} n WHERE n.type = 'blog' AND n.status = 1 ORDER BY n.sticky DESC, n.created DESC"), variable_get('default_nodes_main', 10));
modules/blog/blog.pages.inc:  $result = db_query_range(db_rewrite_sql("SELECT n.nid, n.created FROM {node} n  WHERE n.type = 'blog' AND n.uid = %d AND n.status = 1 ORDER BY n.created DESC"), $account->uid, 0, variable_get('feed_default_items', 10));
modules/blog/blog.pages.inc:  $result = db_query_range(db_rewrite_sql("SELECT n.nid, n.created FROM {node} n WHERE n.type = 'blog' AND n.status = 1 ORDER BY n.created DESC"), 0, variable_get('feed_default_items', 10));
modules/blogapi/blogapi.module:  $node->status = 1;
modules/book/book.module:      $result2 = db_query(db_rewrite_sql("SELECT n.type, n.title, b.*, ml.* FROM {book} b INNER JOIN {node} n on b.nid = n.nid INNER JOIN {menu_links} ml ON b.mlid = ml.mlid WHERE n.nid IN (". implode(',', $nids) .") AND n.status = 1 ORDER BY ml.weight, ml.link_title"));
modules/comment/comment.module:    $result = db_query_range('SELECT c.nid, c.subject, c.cid, c.timestamp FROM {comments} c INNER JOIN {node} n ON n.nid = c.nid WHERE c.nid IN ('. implode(',', $nids) .') AND n.status = 1 AND c.status = %d ORDER BY c.cid DESC', COMMENT_PUBLISHED, 0, $number);
modules/comment/comment.module.orig:    $result = db_query_range('SELECT c.nid, c.subject, c.cid, c.timestamp FROM {comments} c INNER JOIN {node} n ON n.nid = c.nid WHERE c.nid IN ('. implode(',', $nids) .') AND n.status = 1 AND c.status = %d ORDER BY c.cid DESC', COMMENT_PUBLISHED, 0, $number);
modules/forum/forum.module:            $sql = db_rewrite_sql("SELECT n.nid, n.title, l.comment_count, l.last_comment_timestamp FROM {node} n INNER JOIN {term_node} tn ON tn.nid = n.nid INNER JOIN {term_data} td ON td.tid = tn.tid INNER JOIN {node_comment_statistics} l ON n.nid = l.nid WHERE n.status = 1 AND td.vid = %d ORDER BY l.last_comment_timestamp DESC");
modules/forum/forum.module:            $sql = db_rewrite_sql("SELECT n.nid, n.title, l.comment_count FROM {node} n INNER JOIN {term_node} tn ON tn.nid = n.nid INNER JOIN {term_data} td ON td.tid = tn.tid INNER JOIN {node_comment_statistics} l ON n.nid = l.nid WHERE n.status = 1 AND td.vid = %d ORDER BY n.nid DESC");
modules/forum/forum.module:    $sql = "SELECT r.tid, COUNT(n.nid) AS topic_count, SUM(l.comment_count) AS comment_count FROM {node} n INNER JOIN {node_comment_statistics} l ON n.nid = l.nid INNER JOIN {term_node} r ON n.nid = r.nid WHERE n.status = 1 GROUP BY r.tid";
modules/forum/forum.module:    $sql = "SELECT ncs.last_comment_timestamp, IF (ncs.last_comment_uid != 0, u2.name, ncs.last_comment_name) AS last_comment_name, ncs.last_comment_uid FROM {node} n INNER JOIN {users} u1 ON n.uid = u1.uid INNER JOIN {term_node} tn ON n.vid = tn.vid INNER JOIN {node_comment_statistics} ncs ON n.nid = ncs.nid INNER JOIN {users} u2 ON ncs.last_comment_uid=u2.uid WHERE n.status = 1 AND tn.tid = %d ORDER BY ncs.last_comment_timestamp DESC";
modules/forum/forum.module:  $sql = "SELECT COUNT(n.nid) FROM {node} n INNER JOIN {term_node} tn ON n.vid = tn.vid AND tn.tid = %d LEFT JOIN {history} h ON n.nid = h.nid AND h.uid = %d WHERE n.status = 1 AND n.created > %d AND h.nid IS NULL";
modules/forum/forum.module:  $sql = db_rewrite_sql("SELECT n.nid, r.tid, n.title, n.sticky, u.name, u.uid, n.created AS timestamp, n.comment AS comment_mode, l.last_comment_timestamp, IF(l.last_comment_uid != 0, cu.name, l.last_comment_name) AS last_comment_name, l.last_comment_uid, l.comment_count AS num_comments, f.tid AS forum_tid FROM {node_comment_statistics} l INNER JOIN {node} n ON n.nid = l.nid INNER JOIN {users} cu ON l.last_comment_uid = cu.uid INNER JOIN {term_node} r ON n.vid = r.vid INNER JOIN {users} u ON n.uid = u.uid INNER JOIN {forum} f ON n.vid = f.vid WHERE n.status = 1 AND r.tid = %d");
modules/forum/forum.module:  $sql_count = db_rewrite_sql("SELECT COUNT(n.nid) FROM {node} n INNER JOIN {term_node} r ON n.nid = r.nid AND r.tid = %d WHERE n.status = 1");
modules/forum/forum.module:  $sql = "SELECT n.nid FROM {node} n LEFT JOIN {history} h ON n.nid = h.nid AND h.uid = %d INNER JOIN {term_node} r ON n.nid = r.nid AND r.tid = %d WHERE n.status = 1 AND h.nid IS NULL AND n.created > %d ORDER BY created";
modules/forum/forum.module:  $sql = "SELECT n.nid, n.title, n.sticky, l.comment_count, l.last_comment_timestamp FROM {node} n INNER JOIN {node_comment_statistics} l ON n.nid = l.nid INNER JOIN {term_node} r ON n.nid = r.nid AND r.tid = %d WHERE n.status = 1 ORDER BY n.sticky DESC, ". _forum_get_topic_order_sql(variable_get('forum_order', 1));
modules/forum/forum.module.orig:            $sql = db_rewrite_sql("SELECT n.nid, n.title, l.comment_count, l.last_comment_timestamp FROM {node} n INNER JOIN {term_node} tn ON tn.nid = n.nid INNER JOIN {term_data} td ON td.tid = tn.tid INNER JOIN {node_comment_statistics} l ON n.nid = l.nid WHERE n.status = 1 AND td.vid = %d ORDER BY l.last_comment_timestamp DESC");
modules/forum/forum.module.orig:            $sql = db_rewrite_sql("SELECT n.nid, n.title, l.comment_count FROM {node} n INNER JOIN {term_node} tn ON tn.nid = n.nid INNER JOIN {term_data} td ON td.tid = tn.tid INNER JOIN {node_comment_statistics} l ON n.nid = l.nid WHERE n.status = 1 AND td.vid = %d ORDER BY n.nid DESC");
modules/forum/forum.module.orig:    $sql = "SELECT r.tid, COUNT(n.nid) AS topic_count, SUM(l.comment_count) AS comment_count FROM {node} n INNER JOIN {node_comment_statistics} l ON n.nid = l.nid INNER JOIN {term_node} r ON n.nid = r.nid WHERE n.status = 1 GROUP BY r.tid";
modules/forum/forum.module.orig:    $sql = "SELECT ncs.last_comment_timestamp, IF (ncs.last_comment_uid != 0, u2.name, ncs.last_comment_name) AS last_comment_name, ncs.last_comment_uid FROM {node} n INNER JOIN {users} u1 ON n.uid = u1.uid INNER JOIN {term_node} tn ON n.vid = tn.vid INNER JOIN {node_comment_statistics} ncs ON n.nid = ncs.nid INNER JOIN {users} u2 ON ncs.last_comment_uid=u2.uid WHERE n.status = 1 AND tn.tid = %d ORDER BY ncs.last_comment_timestamp DESC";
modules/forum/forum.module.orig:  $sql = "SELECT COUNT(n.nid) FROM {node} n INNER JOIN {term_node} tn ON n.vid = tn.vid AND tn.tid = %d LEFT JOIN {history} h ON n.nid = h.nid AND h.uid = %d WHERE n.status = 1 AND n.created > %d AND h.nid IS NULL";
modules/forum/forum.module.orig:  $sql = db_rewrite_sql("SELECT n.nid, r.tid, n.title, n.sticky, u.name, u.uid, n.created AS timestamp, n.comment AS comment_mode, l.last_comment_timestamp, IF(l.last_comment_uid != 0, cu.name, l.last_comment_name) AS last_comment_name, l.last_comment_uid, l.comment_count AS num_comments, f.tid AS forum_tid FROM {node_comment_statistics} l INNER JOIN {node} n ON n.nid = l.nid INNER JOIN {users} cu ON l.last_comment_uid = cu.uid INNER JOIN {term_node} r ON n.vid = r.vid INNER JOIN {users} u ON n.uid = u.uid INNER JOIN {forum} f ON n.vid = f.vid WHERE n.status = 1 AND r.tid = %d");
modules/forum/forum.module.orig:  $sql_count = db_rewrite_sql("SELECT COUNT(n.nid) FROM {node} n INNER JOIN {term_node} r ON n.nid = r.nid AND r.tid = %d WHERE n.status = 1");
modules/forum/forum.module.orig:  $sql = "SELECT n.nid FROM {node} n LEFT JOIN {history} h ON n.nid = h.nid AND h.uid = %d INNER JOIN {term_node} r ON n.nid = r.nid AND r.tid = %d WHERE n.status = 1 AND h.nid IS NULL AND n.created > %d ORDER BY created";
modules/forum/forum.module.orig:  $sql = "SELECT n.nid, n.title, n.sticky, l.comment_count, l.last_comment_timestamp FROM {node} n INNER JOIN {node_comment_statistics} l ON n.nid = l.nid INNER JOIN {term_node} r ON n.nid = r.nid AND r.tid = %d WHERE n.status = 1 ORDER BY n.sticky DESC, ". _forum_get_topic_order_sql(variable_get('forum_order', 1));
modules/node/node.module:      $total = db_result(db_query('SELECT COUNT(*) FROM {node} WHERE status = 1'));
modules/node/node.module:      $conditions1 = 'n.status = 1';
modules/node/node.module:    $result = db_query_range(db_rewrite_sql('SELECT n.nid, n.created FROM {node} n WHERE n.promote = 1 AND n.status = 1 ORDER BY n.created DESC'), 0, variable_get('feed_default_items', 10));
modules/node/node.module:  $result = pager_query(db_rewrite_sql('SELECT n.nid, n.sticky, n.created FROM {node} n WHERE n.promote = 1 AND n.status = 1 ORDER BY n.sticky DESC, n.created DESC'), variable_get('default_nodes_main', 10));
modules/node/node.module:  $node->status = 1;
modules/ping/ping.module:    if (db_result(db_query("SELECT COUNT(*) FROM {node} WHERE status = 1 AND (created > '". variable_get('cron_last', time()) ."' OR changed > '". variable_get('cron_last', time()) ."')"))) {
modules/poll/poll.module:      $sql = db_rewrite_sql("SELECT MAX(n.created) FROM {node} n INNER JOIN {poll} p ON p.nid = n.nid WHERE n.status = 1 AND p.active = 1");
modules/poll/poll.module.orig:      $sql = db_rewrite_sql("SELECT MAX(n.created) FROM {node} n INNER JOIN {poll} p ON p.nid = n.nid WHERE n.status = 1 AND p.active = 1");
modules/poll/poll.pages.inc:  $sql = db_rewrite_sql("SELECT n.nid, n.title, p.active, n.created, SUM(c.chvotes) AS votes FROM {node} n INNER JOIN {poll} p ON n.nid = p.nid INNER JOIN {poll_choices} c ON n.nid = c.nid WHERE n.status = 1 GROUP BY n.nid, n.title, p.active, n.created ORDER BY n.created DESC");
modules/poll/poll.pages.inc:  $count_sql = db_rewrite_sql('SELECT COUNT(*) FROM {node} n INNER JOIN {poll} p ON n.nid = p.nid WHERE n.status = 1');
modules/statistics/statistics.module:    return db_query_range(db_rewrite_sql("SELECT n.nid, n.title, u.uid, u.name FROM {node} n INNER JOIN {node_counter} s ON n.nid = s.nid INNER JOIN {users} u ON n.uid = u.uid WHERE s.". $dbfield ." != 0 AND n.status = 1 ORDER BY s.". $dbfield ." DESC"), 0, $dbrows);
modules/system/system.admin.inc:          db_query("UPDATE {system} SET status = 1 WHERE type = 'theme' and name = '%s'", $key);
modules/system/system.admin.inc:    db_query("UPDATE {system} SET status = 1 WHERE type = 'theme' AND name = 'garland'");
modules/system/system.install:    $ret[] = update_sql("UPDATE {system} SET status = 1 WHERE name = 'php' AND type = 'module'");
modules/system/system.install:  $ret[] = update_sql('UPDATE {files} SET status = 1');
modules/taxonomy/taxonomy.module:      $result = db_query(db_rewrite_sql('SELECT t.tid, COUNT(n.nid) AS c FROM {term_node} t INNER JOIN {node} n ON t.vid = n.vid WHERE n.status = 1 GROUP BY t.tid'));
modules/taxonomy/taxonomy.module:      $result = db_query(db_rewrite_sql("SELECT t.tid, COUNT(n.nid) AS c FROM {term_node} t INNER JOIN {node} n ON t.vid = n.vid WHERE n.status = 1 AND n.type = '%s' GROUP BY t.tid"), $type);
modules/taxonomy/taxonomy.module:      $sql = 'SELECT DISTINCT(n.nid), n.sticky, n.title, n.created FROM {node} n INNER JOIN {term_node} tn ON n.vid = tn.vid WHERE tn.tid IN ('. $placeholders .') AND n.status = 1 ORDER BY '. $order;
modules/taxonomy/taxonomy.module:      $sql_count = 'SELECT COUNT(DISTINCT(n.nid)) FROM {node} n INNER JOIN {term_node} tn ON n.vid = tn.vid WHERE tn.tid IN ('. $placeholders .') AND n.status = 1';
modules/taxonomy/taxonomy.module:      $sql = 'SELECT DISTINCT(n.nid), n.sticky, n.title, n.created FROM {node} n '. $joins .' WHERE n.status = 1 '. $wheres .' ORDER BY '. $order;
modules/taxonomy/taxonomy.module:      $sql_count = 'SELECT COUNT(DISTINCT(n.nid)) FROM {node} n '. $joins .' WHERE n.status = 1 '. $wheres;
modules/tracker/tracker.pages.inc:    $sql = 'SELECT DISTINCT(n.nid), n.title, n.type, n.changed, n.uid, u.name, GREATEST(n.changed, l.last_comment_timestamp) AS last_updated, l.comment_count FROM {node} n INNER JOIN {node_comment_statistics} l ON n.nid = l.nid INNER JOIN {users} u ON n.uid = u.uid LEFT JOIN {comments} c ON n.nid = c.nid AND (c.status = %d OR c.status IS NULL) WHERE n.status = 1 AND (n.uid = %d OR c.uid = %d) ORDER BY last_updated DESC';
modules/tracker/tracker.pages.inc:    $sql_count = 'SELECT COUNT(DISTINCT(n.nid)) FROM {node} n LEFT JOIN {comments} c ON n.nid = c.nid AND (c.status = %d OR c.status IS NULL) WHERE n.status = 1 AND (n.uid = %d OR c.uid = %d)';
modules/tracker/tracker.pages.inc:    $sql = 'SELECT DISTINCT(n.nid), n.title, n.type, n.changed, n.uid, u.name, GREATEST(n.changed, l.last_comment_timestamp) AS last_updated, l.comment_count FROM {node} n INNER JOIN {users} u ON n.uid = u.uid INNER JOIN {node_comment_statistics} l ON n.nid = l.nid WHERE n.status = 1 ORDER BY last_updated DESC';
modules/tracker/tracker.pages.inc:    $sql_count = 'SELECT COUNT(n.nid) FROM {node} n WHERE n.status = 1';
update.php:  $query = db_query("SELECT name, type, status FROM {system} WHERE status = 1 AND type IN ('module','theme')");

I don't think something as simple as 0/1 for FALSE/TRUE needs constants very badly.

I was thinking in terms of the menu system, about the use of "(1 = a disabled menu item that may be shown on admin screens, -1 = a menu callback, 0 = a normal, visible link)" in {menu_links}.hidden as a case where the code in menu.inc is confusing because of the magic -1/0/1 system

@Gabor, you are right. SQl queries should be changed also, just like comment.module does with comment status...

$comments = db_query('SELECT subject, comment, format FROM {comments} WHERE nid = %d AND status = %d', $node->nid, COMMENT_PUBLISHED);

@robertgarrigos -- I thiink you are volunteering for a D7 node module cleanup. I think it would be foolish to undertake revising all those (working) queries and code at this stage for D6. I'd only weakly support doing it for menu.inc for the exapmle I cited for D6 just because that it much less obvious in what the "magic" numbers mean.

I have a patch that I had done for the previous 5.x node constants issue as it was marked won't fix, so I had never got around to finishing and uploading it. That patch ended up being a 35KB patch (unfinished), and that's just for 5.x at the time, a current HEAD patch will probably be a bit bigger.

Ok then.

Note that comment module uses the status constants because it has status = 0 for published and status = 1 for unpublished, and it is rather unusual.

@powlanin: you are right at #12 and #14 but not that I'm volunteering (yet) ;-). theborg should say something about it first and assimmonds might almost have it.

Re #17: In Drupal 7 we should probably just switch those around to be standardized and be done with it. Then we can use the same constants across the board.

Reroll to HEAD.

re: #17/#21 comment.module was fixed in a recent commit.

Exact : #237636: Comment status field should match node status field

These are not 'magic numbers'. node.sticky, node.published, node.promoted are (pseudo) booleans. You can read them as:

Is it sticky? Yes/No
Is it published? Yes/No
Is it promoted? Yes/No

What's wrong with that?

Unless you want to take on serious inconsistencies like comments (#17), I'm +1 to won't fix. All that I see on the patch is:

A_B = 1
A_NOT_B = 0

There are already the 'constants' TRUE and FALSE we can use for that.

The problem is the word "can" in the sentence, "You can read them as:"

Anything that is up for interpretation will be interpreted incorrectly at 3 am.

Stuart

+100 kabillion for this.

Tagging as novice. Looks like this was pretty close before it fell off the radar.

An alternative implementation strategy for this would be (as discussed earlier), to simply remove the 3am - 4am timeframe, which is actually very bad for rolling core patches.

Added #428234: Use constants for block visibility. Same issue, but for block attributes.

rerolled to HEAD. I left out the extra columns added by theborg at #19 in order to not interefere with actual interface works. If you are happy with this, I may add other constants found in other core modules, as this patch only acts on node module.

sorry, I missed the patch :-)

I really don't see this as helpful in terms of understanding the code. @webchick - why is this not a "won't fix"?

@pwolanin: You don't see how replacing undocumented magic numbers with named constants improves code readability?

They are not "magic numbers" but booleans, we can add schema support to booleans and change those table fields to booleans (optional) and change code to use TRUE/FALSE instead of 1/0 and ensure they are correct handled by db layer.

@Crell - no I really don't if the only possible values are 1/0

We have had great success replacing 1/0 with constants like COMMENT_PUBLISHED/COMMENT_UNPUBLISHED. This looks good. It isn't complete as far as i can tell, but once it is it is RTBC IMO.

I tend to agree that booleans do not need constants and that comment had it backwads and there it's useful.

However... if I remember correctly comments now are fixed and yet we kept the constants. I am unsure how to proceed... do we want to remove them, then?

Committed to CVS HEAD. Thanks.