[2.0] Fix a bug in Registry.php

[Wang-Yu-Chao]

In class Registry, function set() is supposed to return the old value if exists, according to the comment.
But actually return the newly set value, since assignment is right-associated.

[mbabker]

This needs to be $result = $node->{$nodes[$i]} ?? null; (similar for the array case), as is it's causing unit test failures because of undefined properties.

[mbabker]

If the intent is to return the previously set value, I'd suggest returning null would be the correct behavior. Otherwise now you're in a mixed case where if it wasn't previously set you're getting the newly set value and if it was previously set you're getting the old value and that API inconsistency isn't necessarily a good thing.

[Wang-Yu-Chao]

You are right. But seems that $result = $node->{$nodes[$i]} ?? null cannot pass unit test...strange

[mbabker]

Which is why I said this had to be fixed on the 2.0 branch only. Because the current code behavior, and inherently the tests, all return the newly set value and if the intent is to change it to follow the docs and return the old value, the tests need updating to account for that change in behavior.

[Wang-Yu-Chao]

I see. Thank you for your patience.

[Wang-Yu-Chao]

Should we correct the code and unit tests here or change the document instead?
I only find this returned value used in method setParam in class User, and method onAjaxSampledataApplyStep1 in class PlgSampledataBlog. And seems they don't cause any side effect.

[mbabker]

Personally, I would suggest to change it.

• AbstractApplication::set() is a wrapper around Registry::set() which returns the previous value, it was coded in a way where it bypasses this API inconsistency
• The setters in Language all return the previous value
• Session::set() returns the previous value

So our API design favors setters returning the previous values, with that said though there are some places where our setters have no return as well (i.e. our Uri object). Personally, I favor the return previous value approach, but either way I don't think returning the same value you're setting is correct.

@wilsonge thoughts?

[nibra]

Thank you!