don't understand this evaluation

don't understand this evaluation

by Danny Wahl -
Number of replies: 6

I'm rewriting the renderer for login_info() for a theme but there's one conditional that doesn't make sense to me (I mean I think I understand what it's doing - but I don't know why we're doing that...)

outputrenderers.php #L589

if (is_mnet_remote_user($USER) and $idprovider = $DB->get_record('mnet_host', array('id'=>$USER->mnethostid))) { ... }

What I don't understand is the second part.  Shouldn't $idprovider ALWAYS be instantiated?  It might be the DB record, it might be an error, it might be NULL - but it will always exist, so what's the point, and what's it doing?

Average of ratings: -
In reply to Danny Wahl

Re: don't understand this evaluation

by Dan Poltawski -

It's trying to avoid a database query by taking advantage of short-circuit execution. See http://php.net/manual/en/language.operators.logical.php

If is_mnet_remote_user returns false then the second half of the statement won't get executed because both sides can't possibly be true and the code needing $idprovider won't be called anyway.

See this example:

<?php
echo 'Example:';
if (false and dosomething()) {
    echo 'Got into if statement';
}
echo 'Done.';

function dosomething() {
    echo 'Did Something.';
    return true;
}
Average of ratings: -
In reply to Dan Poltawski

Re: don't understand this evaluation

by Hubert Chathi -

Wouldn't it be clearer to do:

if (is_mnet_remote_user($USER)) {
    $idprovider = $DB->get_record('mnet_host', array('id'=>$USER->mnethostid));
    ...
}

Since $idprovider isn't actually a condition that is being tested (since it should always evaluate to true).

Average of ratings: -
In reply to Hubert Chathi

Re: don't understand this evaluation

by Tim Hunt -
Picture of Core developers Picture of Documentation writers Picture of Particularly helpful Moodlers Picture of Peer reviewers Picture of Plugin developers

I expect $idprovider could be null, if the remote host has been deleted, so you would need a second nested if.

Average of ratings: -
In reply to Tim Hunt

Re: don't understand this evaluation

by Hubert Chathi -

If a user is logged in, but their remote host has been deleted, shouldn't that be an error condition? (so $strictness should probably be set to MUST_EXIST in the get_record call)

Anyways, if $idprovider is indeed being tested, then I agree with putting it in the if condition.  If $idprovider is supposed to always be non-empty, then IMHO it should be outside the if condition.

(BTW, I always liked the convention of putting extra parentheses around variable assignments inside of if conditions, to indicate that an '=' really was intended, and not a '=='.)

Average of ratings: -
In reply to Danny Wahl

Re: don't understand this evaluation

by David Bogner -
Picture of Core developers Picture of Plugin developers
 
 

Hi Danny,

I am just curious, what are you going to override? I did change the login_url for Moodle 2.3, in order to make it possible, that the login url points to the MNET host on remote MNET sites.

In Moodle 2.4 I get some errors, I think it's because $withlinkgs was added in 2.4. What we actually did change, was the get_login_url() function -> theme_get_login_url().

Best regards,

David

function theme_get_login_url(){
    global $PAGE, $DB;
    $authplugin = get_auth_plugin('mnet');
    $authurl = $authplugin->loginpage_idp_list($PAGE->url->out());
    $host = $DB->get_record('mnet_host', array('id' => $PAGE->theme->settings->alternateloginurl),'name');
    if(!empty($authurl)){
        foreach($authurl as $key => $urlarray){
            if($urlarray['name'] == $host->name){
                $loginurl = $authurl[$key]['url'];
            } else {
                $loginurl = get_login_url();
            }
        }
    } else {
        $loginurl = get_login_url();
    }
    return $loginurl;
}

 

 
Average of ratings: -