Use of assign_by_ref() is not clear as to what is required

[Robert Lyon]

In Mahara we have a bunch of $smarty->assign_by_ref('item', $variable);

It was originally added to smarty/dwoo due to the following

"The assign_by_ref() original intention in Smarty 2 was to work around the object-by-copy behavior of PHP4."

"The _by_ref methods have been introduced in Smarty2 mainly to be able to pass objects to the templates in PHP4. In PHP5 these are passed alway as a reference."

But it doesn't look like we use them in a true reference sort of way

What I mean is, this example shows referenced vs not referenced 'title' variable:

$smarty = smarty();
$title = 'cats';
$smarty->assign('title', $title);
$smarty->assign_by_ref('titleref', $title);
$title = 'dogs';
$smarty->display('template.tpl');

In the template it will display 'cat' as title and 'dogs' as titleref rather than 'cat'.

We don't support PHP4 and so should clean up the code and make the assign_by_ref() calls simply assign() where appropriate to make the code clear as to what we are wanting.

[Robert Lyon (robertl-9)]

When I change the lib/web.php line

 $smarty->assign_by_ref('JAVASCRIPT', $javascript_array);

to

 $smarty->assign('JAVASCRIPT', $javascript_array);

The resulting dataroot/dwoo/compile/.../htdocs/mahara-testing/mahara/htdocs/theme/raw/templates/header/head.tpl.d17.php looks to be the same for either option

[Aaron Wells (u-aaronw)]

Here's the function declarations (in htdocs/lib/dwoo/mahara/Dwoo_Mahara.php):

    /**
     * implements smarty api to assign data
     */
    public function assign($key, $value) {
        $this->_data[$key] = $value;
    }

    /**
     * implements smarty api to assign data
     */
    public function assign_by_ref($key, &$value) {
        $this->_data[$key] =& $value;
    }

I guess it makes sense, the difference is that assign_by_ref() passes the parameter by reference (note the "&") and assign() passes it the PHP default way, by value.

There are generally two reasons why you might pass a parameter by reference in PHP. The primary reason is if you want the function to be able to modify its parameters, usually as a way to return multiple values. But in the case of Dwoo, that's unlikely to be useful because we compile these templates last and we only use them for display purposes.

The other reason is for optimization. If a parameter is very large, then you might want to pass it by reference in order to avoid having to make a copy of it. That's probably where PHP 4 comes into this, as well. In PHP 4, passing an object as a parameter would make a copy of the object. In PHP 5 the handling of objects changed, so that variables hold a pointer to the object instead of the actual object-in-memory itself. Consequently, in PHP5 objects passed to functions are already handled rather similarly to if they were passed by reference. (Though not exactly the same. See here for the gory details: http://php.net/manual/en/language.oop5.references.php ). So we don't need $smarty->assign_by_ref() to handle objects anymore.

Additionally, using references for optimization is very 2008. PHP engines from 5+ are optimized so that passing by value is actually *faster* unless you're passing *very* large strings or arrays, and even then it's only a modest speed improvement noticeable if you're doing it thousands of times per page. We have at most a couple dozen $smarty->assign() calls on a page. (See http://stackoverflow.com/questions/178328/in-php-5-0-is-passing-by-reference-faster ) So now the universal advice is to *only* use pass-by-reference if you need to modify parameters, not as an attempt at optimization.

So where does that leave us? Well, since we don't want Dwoo templates modifying the values passed to them, we really should be using $smarty->assign() everywhere instead of $smarty->assign_by_ref(). It probably would be an okay idea to clean up all the old $smarty->assign_by_ref() calls and change them to $smarty->assign(). The one possible exception is if we're passing an enormous array to smarty, like something that could cause an out-of-memory error if it were duplicated. Although even then, it would probably be best to use other means to avoid having that whole array in memory in the first place, like using a file iterator or a database results pointer.

[Mahara Bot (dev-mahara)]

Patch for "master" branch: https://reviews.mahara.org/6945

[Cecilia Vela Gurovic (ceciliavg)]

To test this fix, the results before and after applying the changes should be the same. We need to test that the functionality that uses the data passed by reference (assign_by_ref) is not changed when we use the assign() instead.

[Mahara Bot (dev-mahara)]

Reviewed: https://reviews.mahara.org/6945
Committed: https://git.mahara.org/mahara/mahara/commit/058a00ea784980dc5a148eb4c082785a4485300c
Submitter: Robert Lyon (<email address hidden>)
Branch: master

commit 058a00ea784980dc5a148eb4c082785a4485300c
Author: Cecilia Vela Gurovic <email address hidden>
Date: Thu Sep 8 10:00:09 2016 +1200

Bug 1620416: replace all $smarty->assign_by_ref()
with $smarty->assign()
behatnotneeded

Change-Id: I667463b7732bd3f1dd2619b2836cf4b8c560d264

[Mahara Bot (dev-mahara)]

Patch for "16.10_STABLE" branch: https://reviews.mahara.org/7015

[Mahara Bot (dev-mahara)]

Reviewed: https://reviews.mahara.org/7015
Committed: https://git.mahara.org/mahara/mahara/commit/a53ed4ad580dbf4938115db08ca16690107a83b8
Submitter: Robert Lyon (<email address hidden>)
Branch: 16.10_STABLE

commit a53ed4ad580dbf4938115db08ca16690107a83b8
Author: Cecilia Vela Gurovic <email address hidden>
Date: Thu Sep 8 10:00:09 2016 +1200

Bug 1620416: replace all $smarty->assign_by_ref()
with $smarty->assign()
behatnotneeded

Change-Id: I667463b7732bd3f1dd2619b2836cf4b8c560d264
(cherry picked from commit 058a00ea784980dc5a148eb4c082785a4485300c)