Add `assignByRef` Support

[ssigwart]

@wisskid, it looks like the following works to add back in assignByRef. I didn't fully test the non-local scopes yet, but wanted to get your thoughts on adding this back in before spending too much time on it. What do you think?

[ssigwart]

It looks like something happened to smarty.net, so that's why the tests are failing:

[wisskid]

It looks like something happened to smarty.net, so that's why the tests are failing:

I noticed the same yesterday. Looks like the domain name haa expired. I've contacted Monte to see if we can get it back.

Also, these unit test need to be looked at. I don't think they should fail when a website is down or changes layout.

[wisskid]

@wisskid, it looks like the following works to add back in assignByRef. I didn't fully test the non-local scopes yet, but wanted to get your thoughts on adding this back in before spending too much time on it. What do you think?

My fear is that non local scopes might be an issue 😆

Will have to spend more time on it.

[ssigwart]

My fear is that non local scopes might be an issue 😆

Will have to spend more time on it.

Yep. I'll spend more time on testing those today. Just wanted to make sure you weren't against adding it back in before spending the time on it.

[ssigwart]

@wisskid, I added more tests for each scope. I think they are all working, so I think this is ready for review.

[wisskid]

I'll have a look at it. But to be honest, right now I still doubt if we really need this.

[ssigwart]

But to be honest, right now I still doubt if we really need this.

I was worried about that. That's why I asked before spending the time to update the tests. I really hope it makes it in. I know it's not a feature that should be recommended and at my company, we try to avoid it now, but it's a major hurdle in order to move from version 4 to 5.

[wisskid]

I was worried about that. That's why I asked before spending the time to update the tests. I really hope it makes it in. I know it's not a feature that should be recommended and at my company, we try to avoid it now, but it's a major hurdle in order to move from version 4 to 5.

Why don't you subclass Smarty\Smarty in your code and add the assignByRef function only in your subclass? You could ignore all the complicated scope-stuff and just be like this:

    public function assignByRef($tpl_var, &$value = null, $nocache = false)
    {
         $this->tpl_vars[$tpl_var] = new ByRefVariable($value, $nocache);
    }

You would need to subclass Smarty\Variable into ByRefVariable and only override the constructor there into:

    public function __construct(&$value = null, $nocache = false)
    {
        $this->value = &$value;
        $this->nocache = $nocache;
    }

right?

[ssigwart]

I probably could do that. Since this PR seems to be fully working with scopes too, I'd probably include all the new code. However, I probably won't do this. It would mean I still need to change a ton of type hints in our code to use our new subclass. The worst part though is the risk. In general, I don't think it's a good idea to be subclassing classes in another library. Even if the library has really good release notes, I doubt they'll go into the fine grained details of internal class changes that shouldn't matter to people using it normally, but could break a subclass.

It sounds like the path of least resistance for my company will probably be to lock composer down to version 4.4.1 and fork the library if it comes to that. I get that it's your library and you have the final say on the direction of Smarty. I've been very happy with Smarty for years and was trying to help out by contributing code back. I find it hard to believe that at least a small number of other people aren't going to be affected by the removing of assigning by reference, but at least now they can see a way to hack it if they do choose to upgrade to version 5 and still need assignByRef.

[simplexx]

We use assignByRef in a large application which we are currently updating from v3 to latest version and the removal of this causes pretty big issues because the code contains cases where the assigned value changes after it was assigned.

[wisskid]

@ssigwart I'm considering adding assignByRef back, but still not entirely convinced. I understand that the removal of assignByRef may require changes to your application, but as the documentation for smarty 3 already stated, assignByRef was added in the days of PHP5 and is totally unneeded for "most intents and purposes". My intent for Smarty5 was to remove old clutter and simplify (and improve) the interface to make it easier to use and more stable for maintenance into the future.

You are citing 'pretty big issues', could you explain why it's not feasible to change your application instead. Or use the sub-class approach I suggested above?

[simplexx]

@wisskid for us, it's too late now, we have already started changing the code and spend multiple days, probably weeks on it. with at least another 2-3 full days to go. Yesterday I had to look through a few hundred php files and read through all the code below every single assignByRef statement, to make sure that the assigned php var is not changed. Took me 4-5 hour of the most boring work you could imagine. Now I have a table with around 70-80 occurrences in which the php var is changed after it was assigned. Now another team member has to go through each of them and make sure it will work with assign only.

Essentially it's just a massive waste of time. Our code will not be better (on the contrary, it will be worse and most likely have more bugs). And we gain absolutely nothing from this.

While I understand the intent and I fully agree, I think thar assignByRef is a special case. Removing this has an impact on essentially how applications should be designed from the ground up. And this is where I think it's important to respect smarty's history. I am pretty sure that there are a lot of massive v3 applications out there, still in use, that at some point will need to be updated. and I think that most devs will not be looking through the pull requests section here on github and find the subclass approach. They will just be like: Oh shit, we need to replace assignByRef - and they will start doing it one by one, same as we did before I came to check here.

All in all I think removing this will cause a lot of unnecessary work and more buggy applications.

[wisskid]

@simplexx sorry to hear that. Could you share one or two examples of code where the variable is changed after it was assigned?

[simplexx]

@wisskid sure:

Example 1:

...
                $rlSmarty->assignByRef('account_settings', $account_settings);
                /* get all languages */
                $allLangs = $GLOBALS['languages'];
                $rlSmarty->assign('allLangs', $allLangs);
                if ($_GET['action'] == 'edit') {
                    $i_key = $rlValid->xSql($_GET['type']);
                    if ($i_key == 'agency') {
                        $account_settings[] = array(
                            'key'   => 'force_profile',
                            'name'  => $lang['account_type_force_agency_setup']
                        );
                    }
                    else{
                        $account_settings[] = array(
                            'key'   => 'force_profile',
                            'name'  => $lang['account_type_force_profile']
                        );
                    }
...

Example 2:

...
                /* get account types */
                rlServiceLocator::loadClass('Account');
                $account_types = $rlAccount->getAccountTypes('visitor');
                $rlSmarty->assignByRef('account_types', $account_types);
                if ($_GET['action'] == 'add' || $_GET['action'] == 'edit') {
                    rlServiceLocator::loadClass('Common');
                    rlServiceLocator::loadClass('Categories');
                    rlServiceLocator::loadClass('Mail');
                    rlServiceLocator::loadClass('Resize');
                    rlServiceLocator::loadClass('MembershipPlansAdmin', 'admin');
                    $instances = $GLOBALS['rlCommon']->getInstances();
                    if ($config['membership_module']) {
                        $plans = $rlMembershipPlansAdmin->getPlans();
                        $rlSmarty->assign('plans', $plans);
                    }
                    // Get domain name and scheme
                    $domain = $domain_info['host'];
                    $scheme = $domain_info['scheme'];
                    $rlSmarty->assign('domain', $domain);
                    $rlSmarty->assign('scheme', $scheme);
                    // link to add a membership plan
                    $add_plan_link = RL_URL_HOME . ADMIN . '/index.php?controller=membership_plans&action=add';
                    $rlSmarty->assign('add_plan_link', $add_plan_link);
                    $account_id = $rlValid->xSql($_GET['account']);
                    // get current account info
                    $account_info = $rlAccount->getProfile((int) $account_id);
                    $rlSmarty->assign('aInfo', $account_info);
                    $max_file_size = (int) $GLOBALS['reefless']->getMaxImageSize();
                    $rlSmarty -> assign('max_file_size', $max_file_size);
                    if (!empty($account_info) && $account_info['instance_id'] != RL_INSTANCE) {
                        $account_types = $rlAccount->getAccountTypes('visitor', $account_info['instance_id']);
                    }
...

[ssigwart]

@wisskid, you are referring this a part of the following statement from https://www.smarty.net/docs/en/api.append.by.ref.tpl:

Honestly, I've never paid attention to that or probably even seen it since we started on Smarty 2, which states this on https://www.smarty.net/docsv2/en/api.assign.by.ref.tpl:

The first part of that is exactly what we rely on. As for the Smarty 3 note, it says it's probably not needed, but I don't get the impression that it would be removed. I find it to be a clever trick to be able to assign a variable like an array and have it see added values after the assignment, both in the file and in other files.

I think I've mentioned this before here or in the discussion, but it's not that I can't replace assignByRef or use the subclass approach. It's that both are massive undertakings and quite risky (the subclass being less so, but still pretty risky). We have over 7,600 PHP files, over 2 millions lines of code, and over 5,000 assignments by reference. With the way we assign the references, there's no guarantee that the variable changes after assignByRef will be in the same file either. So trying to update and test all of those is an overwhelming amount of work.

[ssigwart]

I rebased this so that the unit tests pass now that the Smarty website dependency is fixed.