helper.php _get_instructions() possible uninitialized $ins variable?

[fiwswe]

I am not sure whether this case can actually occur and I have not checked what would happen when consumers of the function result get nullinstead of an array but uninitialzed function results seem to me to be bad coding style.

Starting at line 247 of helper.php:

       if ($flags['linkonly']) {
            if (page_exists($page) || $flags['pageexists']  == 0) {
                …
                if($flags['parlink']) {
                     // In this case $ins is not initialized but it is later returned as the function result.
                     … 
                }
        …
        return $ins;
    }

Better coding style would avoid this by initializing $ins = []; at the beginning of the method. Thus later else clauses to this effect can be removed. The whole thing could then look like this:

    function _get_instructions($page, $sect, $mode, $lvl, $flags, $root_id = null, $included_pages = array()) {
        $key = ($sect) ? $page . '#' . $sect : $page;
        $this->includes[$key] = true; // legacy code for keeping compatibility with other plugins
        $ins = [];

        // keep compatibility with other plugins that don't know the $root_id parameter
        if (is_null($root_id)) {
            global $ID;
            $root_id = $ID;
        }

        if ($flags['linkonly']) {
            if (page_exists($page) || $flags['pageexists']  == 0) {
                $title = '';
                if ($flags['title'])
                    $title = p_get_first_heading($page);
                if($flags['parlink']) {
                    $ins = array(
                        array('p_open', array()),
                        array('internallink', array(':'.$key, $title)),
                        array('p_close', array()),
                    );
                } else {
                    $ins = array(array('internallink', array(':'.$key,$title)));
                }
            }
        } else {
            if (page_exists($page)) {
                global $ID;
                $backupID = $ID;
                $ID = $page; // Change the global $ID as otherwise plugins like the discussion plugin will save data for the wrong page
                $ins = p_cached_instructions(wikiFN($page), false, $page);
                $ID = $backupID;
            }

            $this->_convert_instructions($ins, $lvl, $page, $sect, $flags, $root_id, $included_pages);
        }
        return $ins;
    }

Note: I personally like using the [] shorthand style for arrays but feel free to use array() if you like that better.

HTH fiwswe

[fiwswe]

Sorry for the noise!

Rereading the code I see that there is no issue. The $ins return value is always initialized.