Bug 12548 - Auth::checkauth() needs more in-line comments
Summary: Auth::checkauth() needs more in-line comments
Status: CLOSED WONTFIX
Alias: None
Product: Koha
Classification: Unclassified
Component: Authentication (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: David Cook
QA Contact: Testopia
URL:
Keywords:
Depends on: 13499 13521
Blocks:
  Show dependency treegraph
 
Reported: 2014-07-10 03:32 UTC by David Cook
Modified: 2021-12-13 21:10 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description David Cook 2014-07-10 03:32:23 UTC
C4::Auth::checkauth() can be a bit difficult to read. 

Take for instance the following code snippet

unless ($userid || $sessionID) {
    ...
}

That makes it seem like this block will be executed if we haven't sent a userid in the CGI object passed to checkauth().

On the contrary, the userid in the CGI object has been defined as "$q_userid" so this block of code is actually what will be executed when we're logging in with a userid and password.

The $userid mentioned in this condition refers to the "id" retrieved from an existing session or from "$userid = $ENV{'REMOTE_USER'}" (which is an environmental variable set via SIP?).

So that's confusing.

The whitespace is also inconsistent with the actual flow of logic, so that should be fixed up too. 

There are all sorts of little bits that need some explanatory comments.
Comment 1 Martin Renvoize 2014-12-30 19:01:11 UTC
Switched to authentication module as I feel inline code documentaiton should come under the specific module and I wanted it to turn up in my Authentcation Module Maintainer search ;).

I agree Auth.pm needs better inline documentation, and I intend to add it during this cycle given the time.
Comment 2 David Cook 2020-12-23 04:51:47 UTC
While C4::Auth::checkauth() is a nightmare, this bug report doesn't really add value.