You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by "Nicholas Telford (JIRA)" <ji...@apache.org> on 2010/02/25 17:40:27 UTC
[jira] Created: (THRIFT-717) Global variables should not be used
for configuration of PHP library
Global variables should not be used for configuration of PHP library
--------------------------------------------------------------------
Key: THRIFT-717
URL: https://issues.apache.org/jira/browse/THRIFT-717
Project: Thrift
Issue Type: Improvement
Components: Library (PHP)
Reporter: Nicholas Telford
The Thrift PHP library makes gratuitous use of the $GLOBALS array to store basic configuration.
Globals in PHP are generally bad practice, so I suggest something else: Use constants.
Being immutable, constants are more secure than globals (that could be overwritten in scripts susceptible to injection attacks); they also perform much better, since the $GLOBALS variable is a hash-table, lookups are comparatively expensive.
I will attach a patch soon unless anyone has any better ideas.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (THRIFT-717) Global variables should not be used
for configuration of PHP library
Posted by "Nicholas Telford (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-717?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Nicholas Telford updated THRIFT-717:
------------------------------------
Attachment: thrift_php_globalsToConstants.diff
Patch to change the $GLOBALS['THRIFT_ROOT'] global variable to the THRIFT_ROOT constant.
> Global variables should not be used for configuration of PHP library
> --------------------------------------------------------------------
>
> Key: THRIFT-717
> URL: https://issues.apache.org/jira/browse/THRIFT-717
> Project: Thrift
> Issue Type: Improvement
> Components: Library (PHP)
> Reporter: Nicholas Telford
> Attachments: thrift_php_globalsToConstants.diff
>
> Original Estimate: 2h
> Remaining Estimate: 2h
>
> The Thrift PHP library makes gratuitous use of the $GLOBALS array to store basic configuration.
> Globals in PHP are generally bad practice, so I suggest something else: Use constants.
> Being immutable, constants are more secure than globals (that could be overwritten in scripts susceptible to injection attacks); they also perform much better, since the $GLOBALS variable is a hash-table, lookups are comparatively expensive.
> I will attach a patch soon unless anyone has any better ideas.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-717) Global variables should not be used
for configuration of PHP library
Posted by "David Reiss (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-717?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12839780#action_12839780 ]
David Reiss commented on THRIFT-717:
------------------------------------
Constants pollute a different global namespace, so I don't see the win. Plus, the likelihood of a collision with "THRIFT_ROOT" is small. Yes, immutability protects you from another library messing you up by running later, but exposes you to another library messing you up by running sooner, so I don't see the win. Plus, it reduces your flexibility with how you choose to set THRIFT_ROOT.
Yes, there are references in the generated code, created by the compiler.
> Global variables should not be used for configuration of PHP library
> --------------------------------------------------------------------
>
> Key: THRIFT-717
> URL: https://issues.apache.org/jira/browse/THRIFT-717
> Project: Thrift
> Issue Type: Improvement
> Components: Library (PHP)
> Reporter: Nicholas Telford
> Priority: Minor
> Attachments: thrift_php_globalsToConstants.diff
>
> Original Estimate: 2h
> Remaining Estimate: 2h
>
> The Thrift PHP library makes gratuitous use of the $GLOBALS array to store basic configuration.
> Globals in PHP are generally bad practice, so I suggest something else: Use constants.
> Being immutable, constants are more secure than globals (that could be overwritten in scripts susceptible to injection attacks); they also perform much better, since the $GLOBALS variable is a hash-table, lookups are comparatively expensive.
> I will attach a patch soon unless anyone has any better ideas.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (THRIFT-717) Global variables should not be used
for configuration of PHP library
Posted by "Nicholas Telford (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-717?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Nicholas Telford updated THRIFT-717:
------------------------------------
Priority: Minor (was: Major)
> Global variables should not be used for configuration of PHP library
> --------------------------------------------------------------------
>
> Key: THRIFT-717
> URL: https://issues.apache.org/jira/browse/THRIFT-717
> Project: Thrift
> Issue Type: Improvement
> Components: Library (PHP)
> Reporter: Nicholas Telford
> Priority: Minor
> Attachments: thrift_php_globalsToConstants.diff
>
> Original Estimate: 2h
> Remaining Estimate: 2h
>
> The Thrift PHP library makes gratuitous use of the $GLOBALS array to store basic configuration.
> Globals in PHP are generally bad practice, so I suggest something else: Use constants.
> Being immutable, constants are more secure than globals (that could be overwritten in scripts susceptible to injection attacks); they also perform much better, since the $GLOBALS variable is a hash-table, lookups are comparatively expensive.
> I will attach a patch soon unless anyone has any better ideas.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-717) Global variables should not be used
for configuration of PHP library
Posted by "Chris Goffinet (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-717?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12838491#action_12838491 ]
Chris Goffinet commented on THRIFT-717:
---------------------------------------
I agree, there is no win here.
> Global variables should not be used for configuration of PHP library
> --------------------------------------------------------------------
>
> Key: THRIFT-717
> URL: https://issues.apache.org/jira/browse/THRIFT-717
> Project: Thrift
> Issue Type: Improvement
> Components: Library (PHP)
> Reporter: Nicholas Telford
> Priority: Minor
> Attachments: thrift_php_globalsToConstants.diff
>
> Original Estimate: 2h
> Remaining Estimate: 2h
>
> The Thrift PHP library makes gratuitous use of the $GLOBALS array to store basic configuration.
> Globals in PHP are generally bad practice, so I suggest something else: Use constants.
> Being immutable, constants are more secure than globals (that could be overwritten in scripts susceptible to injection attacks); they also perform much better, since the $GLOBALS variable is a hash-table, lookups are comparatively expensive.
> I will attach a patch soon unless anyone has any better ideas.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-717) Global variables should not be used
for configuration of PHP library
Posted by "Nicholas Telford (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-717?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12839664#action_12839664 ]
Nicholas Telford commented on THRIFT-717:
-----------------------------------------
Ok, sure, the performance and security issues are fairly pointless. The biggest problem here is pollution of the global namespace, which is a bit of a show-stopper when integrating in to larger frameworks or libraries.
Constants work because they don't pollute the $GLOBALS super-global and are immutable, so other system components can't accidentally modify them (i.e. through naming clashes) without the developer being alerted to it.
I thought I'd got all references to $GLOBALS['THRIFT_ROOT'], I take it some reside outside of lib/php/src/ ?
> Global variables should not be used for configuration of PHP library
> --------------------------------------------------------------------
>
> Key: THRIFT-717
> URL: https://issues.apache.org/jira/browse/THRIFT-717
> Project: Thrift
> Issue Type: Improvement
> Components: Library (PHP)
> Reporter: Nicholas Telford
> Priority: Minor
> Attachments: thrift_php_globalsToConstants.diff
>
> Original Estimate: 2h
> Remaining Estimate: 2h
>
> The Thrift PHP library makes gratuitous use of the $GLOBALS array to store basic configuration.
> Globals in PHP are generally bad practice, so I suggest something else: Use constants.
> Being immutable, constants are more secure than globals (that could be overwritten in scripts susceptible to injection attacks); they also perform much better, since the $GLOBALS variable is a hash-table, lookups are comparatively expensive.
> I will attach a patch soon unless anyone has any better ideas.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-717) Global variables should not be used
for configuration of PHP library
Posted by "James Bathgate (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-717?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12838414#action_12838414 ]
James Bathgate commented on THRIFT-717:
---------------------------------------
+1
> Global variables should not be used for configuration of PHP library
> --------------------------------------------------------------------
>
> Key: THRIFT-717
> URL: https://issues.apache.org/jira/browse/THRIFT-717
> Project: Thrift
> Issue Type: Improvement
> Components: Library (PHP)
> Reporter: Nicholas Telford
> Original Estimate: 2h
> Remaining Estimate: 2h
>
> The Thrift PHP library makes gratuitous use of the $GLOBALS array to store basic configuration.
> Globals in PHP are generally bad practice, so I suggest something else: Use constants.
> Being immutable, constants are more secure than globals (that could be overwritten in scripts susceptible to injection attacks); they also perform much better, since the $GLOBALS variable is a hash-table, lookups are comparatively expensive.
> I will attach a patch soon unless anyone has any better ideas.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-717) Global variables should not be used
for configuration of PHP library
Posted by "David Reiss (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-717?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12838463#action_12838463 ]
David Reiss commented on THRIFT-717:
------------------------------------
Well, I can't accept this patch because it doesn't update every use of THRIFT_ROOT. However, I'm not convinced that this would be a good approach even if the patch were complete. I think the security argument is completely pointless, since any attacker capable of injecting PHP code into your execution would not need to mess with Thrift to take over your server. The performance argument is also not compelling, since the cost of a single global lookup is tiny compared to the cost of including a library file (even with APC (or HipHop)). Finally, I think that leaving the value in a global gives users more freedom when setting up their environment. It is easy to determine if it is already set and easy to fix after-the-fact if you need to hack around something in your sandbox.
> Global variables should not be used for configuration of PHP library
> --------------------------------------------------------------------
>
> Key: THRIFT-717
> URL: https://issues.apache.org/jira/browse/THRIFT-717
> Project: Thrift
> Issue Type: Improvement
> Components: Library (PHP)
> Reporter: Nicholas Telford
> Priority: Minor
> Attachments: thrift_php_globalsToConstants.diff
>
> Original Estimate: 2h
> Remaining Estimate: 2h
>
> The Thrift PHP library makes gratuitous use of the $GLOBALS array to store basic configuration.
> Globals in PHP are generally bad practice, so I suggest something else: Use constants.
> Being immutable, constants are more secure than globals (that could be overwritten in scripts susceptible to injection attacks); they also perform much better, since the $GLOBALS variable is a hash-table, lookups are comparatively expensive.
> I will attach a patch soon unless anyone has any better ideas.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (THRIFT-717) Global variables should not be used
for configuration of PHP library
Posted by "Nicholas Telford (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-717?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Nicholas Telford updated THRIFT-717:
------------------------------------
Patch Info: [Patch Available]
Added patch to change the THRIFT_ROOT to a constant.
It can be defined manually, or intelligently derived by including the base Thrift.php:
define('THRIFT_ROOT', '/path/to/thrift/lib/php/src');
require_once THRIFT_ROOT.'/Thrift.php';
// or
require_once '/path/to/thrift/lib/php/src/Thrift.php';
> Global variables should not be used for configuration of PHP library
> --------------------------------------------------------------------
>
> Key: THRIFT-717
> URL: https://issues.apache.org/jira/browse/THRIFT-717
> Project: Thrift
> Issue Type: Improvement
> Components: Library (PHP)
> Reporter: Nicholas Telford
> Attachments: thrift_php_globalsToConstants.diff
>
> Original Estimate: 2h
> Remaining Estimate: 2h
>
> The Thrift PHP library makes gratuitous use of the $GLOBALS array to store basic configuration.
> Globals in PHP are generally bad practice, so I suggest something else: Use constants.
> Being immutable, constants are more secure than globals (that could be overwritten in scripts susceptible to injection attacks); they also perform much better, since the $GLOBALS variable is a hash-table, lookups are comparatively expensive.
> I will attach a patch soon unless anyone has any better ideas.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-717) Global variables should not be used
for configuration of PHP library
Posted by "Todd Lipcon (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-717?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12838487#action_12838487 ]
Todd Lipcon commented on THRIFT-717:
------------------------------------
I agree with David on all points above. the globals lookup is meaningless for performance next to the multi-millisecond RPC you're about to send out :)
> Global variables should not be used for configuration of PHP library
> --------------------------------------------------------------------
>
> Key: THRIFT-717
> URL: https://issues.apache.org/jira/browse/THRIFT-717
> Project: Thrift
> Issue Type: Improvement
> Components: Library (PHP)
> Reporter: Nicholas Telford
> Priority: Minor
> Attachments: thrift_php_globalsToConstants.diff
>
> Original Estimate: 2h
> Remaining Estimate: 2h
>
> The Thrift PHP library makes gratuitous use of the $GLOBALS array to store basic configuration.
> Globals in PHP are generally bad practice, so I suggest something else: Use constants.
> Being immutable, constants are more secure than globals (that could be overwritten in scripts susceptible to injection attacks); they also perform much better, since the $GLOBALS variable is a hash-table, lookups are comparatively expensive.
> I will attach a patch soon unless anyone has any better ideas.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.