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.