Ordering by 'Visitor' in access log pages does not sort IP addresses.

the query is getting both the username and the hostname (ip). all sorts are done on the username field only. as he prepares to display the results of the query, he swaps in the hostname if the username is empty. so the hostnames are never really sorted at all (and probably shouldn't be values in a sortable field :)

$rows[] = array($account->hits, ($account->uid ? theme('username', $account) : $account->hostname), format_interval(round($account->total / 1000)), $ban_link);

This looks more like a feature request, and affects HEAD.

One solution to this problem would require to store IPs using a different format, maybe with the same approach implemented by phpBB. It uses CHAR(8) in the database, and converts the IP address to an hexadecimal value, where each part needs 2 bytes.

They use this functions:

function encode_ip($dotquad_ip) {
  $ip_sep = explode('.', $dotquad_ip);
  return sprintf('%02x%02x%02x%02x', $ip_sep[0], $ip_sep[1], $ip_sep[2], $ip_sep[3]);
}
function decode_ip($int_ip) {
  $hexipbang = explode('.', chunk_split($int_ip, 2, '.'));
  return hexdec($hexipbang[0]). '.' . hexdec($hexipbang[1]) . '.' . hexdec($hexipbang[2]) . '.' . hexdec($hexipbang[3]);
}

That way IPs can be sorted on a more logical manner and is more optimal than converting IPs to longs.

Forgot to mention side effects related to such a change...

1) Stuff related to {access_log} table (statistics module).
2) Stuff related to {access} table (ip bans, etc.).
3) Watchdog.
4) Probably some contrib modules too.

I don't believe this will be done any time soon... and I can't think of any other reasonable way to sort IPs logically.

Did current versions taken this feature request in consideration?

Moving to current.

Needs to be fixed first in D8.

WOW!! A seven-year-old issue... lets see if we cant just kill this one.

The attached patch fixes this "enough" IMO by always sorting the ip (ASC), but since this particular column in this particular table actually contains data from two DB columns (name & ip) it does not obey the tablesort link in the header.

I think this is an acceptable solution in this case though. If we want we can open up a feature request for orderByHeader() to accept multiple fields, but that is out of scope for this issue me thinks.

Looks good to me, a very simple solution to the issue that has the possibility to get back-ported to D7 too.

Although, my preference would be to remove all of the access_log functionality from D8 and just keep the node_counter.

There's no index on hostname, could we get a before/after EXPLAIN for the query with the default sort? 'needs profiling' isn't quite the right tag but in the interests of not creating a new tag, adding that.

The explains are identical in all cases. Adding an index on accesslog.hostname did not alter the way either of these queries was executed. If we're sorting based on a calculated value like hits, I think there's no way to avoid using a temporary table.

Marking this as RTBC based on #9.

Without patch:

mysql> EXPLAIN SELECT a.uid AS uid, a.hostname AS hostname, u.name AS name, COUNT( a.uid ) AS hits, SUM( a.timer ) AS total FROM  `accesslog` a LEFT OUTER JOIN users u ON a.uid = u.uid GROUP BY a.hostname, a.uid, u.name ORDER BY hits DESC;
+----+-------------+-------+--------+---------------+---------+---------+----------+------+---------------------------------+
| id | select_type | table | type   | possible_keys | key     | key_len | ref      | rows | Extra                           |
+----+-------------+-------+--------+---------------+---------+---------+----------+------+---------------------------------+
|  1 | SIMPLE      | a     | ALL    | NULL          | NULL    | NULL    | NULL     |   34 | Using temporary; Using filesort |
|  1 | SIMPLE      | u     | eq_ref | PRIMARY       | PRIMARY | 4       | d8.a.uid |    1 |                                 |
+----+-------------+-------+--------+---------------+---------+---------+----------+------+---------------------------------+

Patched:

mysql> EXPLAIN SELECT a.uid AS uid, a.hostname AS hostname, u.name AS name, COUNT(a.uid) AS hits, SUM(a.timer) AS total FROM accesslog a LEFT OUTER JOIN users u ON a.uid = u.uid GROUP BY a.hostname, a.uid, u.name ORDER BY hits DESC, a.hostname ASC;
+----+-------------+-------+--------+---------------+---------+---------+----------+------+---------------------------------+
| id | select_type | table | type   | possible_keys | key     | key_len | ref      | rows | Extra                           |
+----+-------------+-------+--------+---------------+---------+---------+----------+------+---------------------------------+
|  1 | SIMPLE      | a     | ALL    | NULL          | NULL    | NULL    | NULL     |   34 | Using temporary; Using filesort |
|  1 | SIMPLE      | u     | eq_ref | PRIMARY       | PRIMARY | 4       | d8.a.uid |    1 |                                 |
+----+-------------+-------+--------+---------------+---------+---------+----------+------+---------------------------------+

Sorry guys, I would rather see accesslog removed #1446956: Remove the accesslog from statistics

Which was just committed.

I think Dries trumped this issue in D8 :) - http://drupal.org/node/1446956#comment-6952152.

But we can still fix this in D7.

Backported #8 to D7.

#15: 28175-15-access-log-sort.patch queued for re-testing.

Test went fatal on "ShortcutSetsTestCase->testShortcutSetDeleteDefault()". Seems outside of the scope of the patch thus retesting it.

I tested this on simplytest.me by running some requests through a proxy service and reviewing the order of IP addresses shown. Looks good in D7. Code is fine too, patch still applies.

test forum pager fail?

15: 28175-15-access-log-sort.patch queued for re-testing.

15: 28175-15-access-log-sort.patch queued for re-testing.

15: 28175-15-access-log-sort.patch queued for re-testing.

Re-setting RTBC status after random test failure.

15: 28175-15-access-log-sort.patch queued for re-testing.

Re-setting RTBC status after random test failure.

Committed to 7.x - thanks!