forbiddance of ini_set('include_path', ...) function causes fatal error at revision feature

[codestyling]

The newly introduced revision feature uses PHP ini_set() function but must not. Several provider doesn't permit setting php.ini values especially the include_path, which should be temporary switched during load of Text/Diff files. The fatal error resulting looks like this out of real customer occurance:

Warning: ini_set() has been disabled for security reasons in /srv/www/httpd/phost/l/de/pytalhost/laz/web/wordpress/wp-includes/pluggable.php on line 1517
Warning: require_once(Text/Diff/Renderer.php) [function.require-once]: failed to open stream: No such file or directory in /srv/www/httpd/phost/l/de/pytalhost/laz/web/wordpress/wp-includes/Text/Diff/Renderer/inline.php on line 17
Fatal error: require_once() [function.require]: Failed opening required ‘Text/Diff/Renderer.php’ (include_path=’.:/srv/www/httpd/phost/l/de/pytalhost/laz/web’) in /srv/www/httpd/phost/l/de/pytalhost/laz/web/wordpress/wp-includes/Text/Diff/Renderer/inline.php on line 17

This error affects all editor pages won't longer behave as expected. Moreover some provider doesn't show any errors and silently abort the engine. This results in white pages served!
I have attached a patch suggestion (containing 4 file from WP 2.6.0) which doesn't use ini_set() anymore but all require calls now using _ _FILE_ _ evaluation of their appropriated complete file path. Has been successful tested at this affected system. Can also be simulated, if you comment the ini_set() inside pluggable.php function wp_text_diff().

[santosj]

Could use set_include_path() function instead.

[santosj]

Can you create a new file with <?php phpinfo();

and report what disable_functions setting has?

[codestyling]

At the moment those functions are disabled:
exec,passthru,system,shell_exec,proc_open,proc_get_status,proc_close,escapeshellcmd,es capeshellarg,ini_set,ini_restore

[azaozz]

Perhaps set_include_path() would work here and if not, can show error message instead.

[jacobsantos]

You have a typo in the message.

[jacobsantos]

Use set_include_path() and restore_include_path() based off of r8483

[jacobsantos]

Should probably return WP_Error() instead of a string. However, I'm not going to dig down into the wp-admin structure in order to make that work.

[azaozz]

Replying to jacobsantos:

Should probably return WP_Error() instead of a string. However, I'm not going to dig down into the wp-admin structure in order to make that work.

Yes, was thinking to return WP_Error and change revision.php so it can handle errors.

Don't think a conditional is needed here. If a host has disabled set_include_path() why would they leave ini_set() enabled. Also what's the purpose of running restore_include_path() at this point? The include path may have been extended before wp_text_diff() was called and restoring it to default may break another function.

[jacobsantos]

Good Points. Having a fall back is never a bad thing to have and with most instances, the first conditional will evaluate to true. For edge cases where ini_set() is enabled, but set_include_path() is disabled. It seems unlikely, but available.

I don't like error suppression, because you really don't gain anything from it, the error will still be passed to either the Apache error log, so no benefit from that. Also before PHP 5.2, it is quite the performance hit.

If you want to remove restore_include_path() then it is possible. I had it because it already exists within the function. If it currently works, then there is no harm, but there is no harm in keeping it either.

[santosj]

This should be in the wp-settings.php. It should create a constant named WP_INCLUDE_PATH_DISABLED, in which case, the include path appears to be set each time the function is called. If it is called once, then no big deal, if it is called 20 times, then the include path is set and then restored 20 times. This adds overhead to the process. It is better to do it once outside of the function.

[codestyling]

I don't understand the strict order "not modify affected files". These files original comes from Pear Framework and are just a copy out of to be present at WP. So WP may maintain now these copies as the would be part of the product itself.
In this case, these files can be modified to required the needed other ones by using the correct path an not be dependend of other ini manipulation function potential disabled by hoster too. Doing so, not further errors will occure here.
The alternative would be to define WP needs PHP5 (at some hoster it doesn't run any longer with PHP4, i proofed this!) and the classes will be loaded by autoload feature of PHP. Why working around several better ways fo just keeping this few files as is ?

[santosj]

Replying to codestyling:

I don't understand the strict order "not modify affected files".

The reason is that when you go to upgrade, you have to keep making those modifications again, and again. This is not ideal.

[jacobsantos]

What is needed for the patch?

[santosj]

Patch for trunk (r8561) sets the include path in wp-settings if Revisions aren't disabled.

[santosj]

Can you check the new patch and see if it is acceptable?

[santosj]

Set include path in wp-settings for 2.6 r8561

[santosj]

Patch for 2.6 also.

[azaozz]

Fixed in [8562] and [8563].

[azaozz]

Reverted... Patch throws a fatal error.

[ryan]

7391.r8579: codestyling's changes as a diff.

[ryan]

(In [8581]) Fix Text/Diff includes. Props codestyling. fixes #7391 for trunk

[ryan]

(In [8582]) Fix Text/Diff includes. Props codestyling. fixes #7391 for 2.6