Problem/Motivation

The database query in login_history_last_login() doesn't have an orderBy clause which means that the database engine used is responsible for the order. Since the query is further limited by a range(0,1) to get only one result, this means that this result is in no way assured to be the desired value. It seems that in MySQL's InnoDB, the order will be the one of the primary key ('login_id'), which is probably 99% of the usage base out there.

Also, a minor issue is that fetchAll() is being used, followed by a reset(). There's no point in doing this, as you can also easily just return the value of fetch().

Steps to reproduce

Look at the code.

Proposed resolution

Define an explicit order using orderBy('login', 'DESC'), and use fetch() instead of fetchAll().

Remaining tasks

Implement it.

Comments

jcnventura created an issue.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented

Just documenting from slack conversation that jcnventura and I discussed this and agreed with the proposed logic change. The original implementation was probably what worked with minimal test set and is not the logically correct behavior.

jcnventura’s picture

jcnventura
Primary language English
commented

I think I also found out why in the beginning this function was offset from 1, instead of 0. The last login block, which reports information on the previous login uses this data, and in reality wants the next to last login (offset 1) instead of the last login (offset 0).. This means that the block is probably broken in the D7 version of the module. The function is also used in the email notification which probably wants the offset 0 currently set.

I've decided to deprecate this function, and copied the code to the block plugin in the D8 version. See #3185870: Use the database API in login_history_last_login and deprecate it.