Skip to content

Revert passing script variables by reference #16030

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Feb 3, 2022

Conversation

Mark-H
Copy link
Collaborator

@Mark-H Mark-H commented Feb 3, 2022

What does it do?

#15578 added the EXTR_REFS flag to the extract() function in modScript::process with the intention of allowing plugins to manipulated variables passed in to it.

This PR reverts that back to how it was in 2.x.

Why is it needed?

Unfortunately per https://community.modx.com/t/spform-how-to-change-language/4917/16 the pass by reference is causing issues when local variables in a snippet share the name of a script property or a copy of $scriptProperties is made. This is causing wildly unexpected results.

How to test

Two very simple snippets to reproduce the behavior are provided in the forum, both assuming the snippet is called with [[!mySnippet? &my_prop=`A`]]:

<?php
$my_prop = 'B';
return $scriptProperties['my_prop']; // returns 'B'
<?php
$newArr = $scriptProperties;
$newArr['my_prop'] = 'B';

return $scriptProperties['my_prop']; // returns 'B'

The expected result, because there is no explicit change made to $scriptProperties, is A. However both return B.

It also goes the other way, though this type of code would be less common:

<?php
$scriptProperties['my_prop'] = 'B';
return $my_prop;

Related issue(s)/PR(s)

https://community.modx.com/t/spform-how-to-change-language/4917/15
#15578

modxcms#15578 added the EXTR_REFS flag to the extract() function in modScript::process with the intention of allowing plugins to manipulated variables passed in to it.

Unfortunately per https://community.modx.com/t/spform-how-to-change-language/4917/16 this is causing issues when local variables in a snippet share the name of a script property or a copy of $scriptProperties is made. This is causing wildly unexpected results.
@Mark-H Mark-H requested a review from opengeek as a code owner February 3, 2022 13:31
@cla-bot cla-bot bot added the cla-signed CLA confirmed for contributors to this PR. label Feb 3, 2022
@Mark-H Mark-H added the urgent The issue requires attention and has higher priority over others. label Feb 3, 2022
@Mark-H Mark-H added this to the v3.0.0-rc2 milestone Feb 3, 2022
@Mark-H Mark-H requested a review from sergant210 February 3, 2022 13:34
@rthrash
Copy link
Member

rthrash commented Feb 3, 2022

This pull request has been mentioned on MODX Community. There might be relevant details there:

https://community.modx.com/t/spform-how-to-change-language/4917/19

@opengeek opengeek merged commit f96ad3e into modxcms:3.x Feb 3, 2022
@BobRay
Copy link
Contributor

BobRay commented Feb 3, 2022

IMO, this should be applied. Passing $scriptProperties by reference is asking for trouble, since many plugins and snippets use variable names that are in the $sccriptProperties array. I suspect that it also introduces opportunities for malicious code. since any variable in the $scriptProperties array could be modified.

@opengeek
Copy link
Member

opengeek commented Feb 3, 2022

@BobRay — It is already applied and live in the RC2 download.

@BobRay
Copy link
Contributor

BobRay commented Feb 3, 2022

Thanks. I should have checked.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed CLA confirmed for contributors to this PR. urgent The issue requires attention and has higher priority over others.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants