External Libraries: Adopt standard get_temp_dir() in Text_Diff#9276
External Libraries: Adopt standard get_temp_dir() in Text_Diff#9276Krinkle wants to merge 1 commit intoWordPress:trunkfrom
Conversation
This is placing files in a potentially different place than the rest of WordPress, and its potential false return value was not checked by the only caller in Text_Diff_Engine_shell. I considered changing our get_temp_dir implementations to check the additional directories that Text_Diff::_getTempDir considered, but decided against it. The Text_Diff code predates the introduction of sys_get_temp_dir in PHP 5.2, and so is checking these to make up for that. The latest version of the code that this was based on no longer does this and calls sys_get_temp_dir instead. Specifically, it removed _getTempDir() in favor of tempnam(null) [1], and later switched to horde/util, [2] which uses sys_get_temp_dir. [3] [1]: horde/Text_Diff@323c4783b9 [2]: horde/Text_Diff@152f6aacf8 [3]: https://github.com/horde/Util/blob/v2.5.12/lib/Horde/Util.php#L200
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
apermo
left a comment
There was a problem hiding this comment.
LGTM, maybe we could consider to mark _getTempDir() as deprecated altogether? And use suggest to directly use get_temp_dir()
This was placing files in a potentially different place than the rest of WordPress, and its potential false return value was not checked by the only caller in
Text_Diff_Engine_shell.I considered changing our
get_temp_dirimplementations to check the additional directories thatText_Diff::_getTempDirconsidered, but decided against it. The Text_Diff code predates the introduction of sys_get_temp_dir in PHP 5.2, and so likely only checking these to make up for that. The latest upstream version of the code that this is based on, also no longer does this and simply calls sys_get_temp_dir instead. Specifically, it removed_getTempDirin favor oftempnam(null)1, and later switched to the horde/util package, 2 which usessys_get_temp_dir. 3Trac ticket: https://core.trac.wordpress.org/ticket/63711
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.