-
Notifications
You must be signed in to change notification settings - Fork 88
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
A first approach at fixing s9y for php 7, which makes it possible to write an entry without any error message. The specific changes are: 1. __construct for the plugin classes 2. Update Cache Lite to a modern version to fix its similar constructor problem 3. Remove the session_regenerate_id call from the session destructor (should get re-added to session creation where necessary) 4. Remove error handler to prevent silenced warnings from becoming fatal exceptions
- Loading branch information
Showing
9 changed files
with
219 additions
and
208 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,90 @@ | ||
| <?php | ||
|
|
||
| /** | ||
| * This class extends Cache_Lite and offers a cache system driven by a master file | ||
| * | ||
| * With this class, cache validity is only dependent of a given file. Cache files | ||
| * are valid only if they are older than the master file. It's a perfect way for | ||
| * caching templates results (if the template file is newer than the cache, cache | ||
| * must be rebuild...) or for config classes... | ||
| * There are some examples in the 'docs/examples' file | ||
| * Technical choices are described in the 'docs/technical' file | ||
| * | ||
| * @package Cache_Lite | ||
| * @author Fabien MARTY <fab@php.net> | ||
| */ | ||
|
|
||
| require_once('Cache/Lite.php'); | ||
|
|
||
| class Cache_Lite_File extends Cache_Lite | ||
| { | ||
|
|
||
| // --- Private properties --- | ||
|
|
||
| /** | ||
| * Complete path of the file used for controlling the cache lifetime | ||
| * | ||
| * @var string $_masterFile | ||
| */ | ||
| var $_masterFile = ''; | ||
|
|
||
| /** | ||
| * Masterfile mtime | ||
| * | ||
| * @var int $_masterFile_mtime | ||
| */ | ||
| var $_masterFile_mtime = 0; | ||
|
|
||
| // --- Public methods ---- | ||
|
|
||
| /** | ||
| * Constructor | ||
| * | ||
| * $options is an assoc. To have a look at availables options, | ||
| * see the constructor of the Cache_Lite class in 'Cache_Lite.php' | ||
| * | ||
| * Comparing to Cache_Lite constructor, there is another option : | ||
| * $options = array( | ||
| * (...) see Cache_Lite constructor | ||
| * 'masterFile' => complete path of the file used for controlling the cache lifetime(string) | ||
| * ); | ||
| * | ||
| * @param array $options options | ||
| * @access public | ||
| */ | ||
| function Cache_Lite_File($options = array(NULL)) | ||
| { | ||
| $options['lifetime'] = 0; | ||
| $this->Cache_Lite($options); | ||
| if (isset($options['masterFile'])) { | ||
| $this->_masterFile = $options['masterFile']; | ||
| } else { | ||
| return $this->raiseError('Cache_Lite_File : masterFile option must be set !'); | ||
| } | ||
| if (!($this->_masterFile_mtime = @filemtime($this->_masterFile))) { | ||
| return $this->raiseError('Cache_Lite_File : Unable to read masterFile : '.$this->_masterFile, -3); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Test if a cache is available and (if yes) return it | ||
| * | ||
| * @param string $id cache id | ||
| * @param string $group name of the cache group | ||
| * @param boolean $doNotTestCacheValidity if set to true, the cache validity won't be tested | ||
| * @return string data of the cache (else : false) | ||
| * @access public | ||
| */ | ||
| function get($id, $group = 'default', $doNotTestCacheValidity = false) | ||
| { | ||
| if ($data = parent::get($id, $group, true)) { | ||
| if ($filemtime = $this->lastModified()) { | ||
| if ($filemtime > $this->_masterFile_mtime) { | ||
| return $data; | ||
| } | ||
| } | ||
| } | ||
| return false; | ||
| } | ||
|
|
||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,56 @@ | ||
| <?php | ||
|
|
||
| /** | ||
| * This class extends Cache_Lite and uses output buffering to get the data to cache. | ||
| * It supports nesting of caches | ||
| * | ||
| * @package Cache_Lite | ||
| * @author Markus Tacker <tacker@php.net> | ||
| */ | ||
|
|
||
| require_once('Cache/Lite/Output.php'); | ||
|
|
||
| class Cache_Lite_NestedOutput extends Cache_Lite_Output | ||
| { | ||
| private $nestedIds = array(); | ||
| private $nestedGroups = array(); | ||
|
|
||
| /** | ||
| * Start the cache | ||
| * | ||
| * @param string $id cache id | ||
| * @param string $group name of the cache group | ||
| * @param boolean $doNotTestCacheValidity if set to true, the cache validity won't be tested | ||
| * @return boolean|string false if the cache is not hit else the data | ||
| * @access public | ||
| */ | ||
| function start($id, $group = 'default', $doNotTestCacheValidity = false) | ||
| { | ||
| $this->nestedIds[] = $id; | ||
| $this->nestedGroups[] = $group; | ||
| $data = $this->get($id, $group, $doNotTestCacheValidity); | ||
| if ($data !== false) { | ||
| return $data; | ||
| } | ||
| ob_start(); | ||
| ob_implicit_flush(false); | ||
| return false; | ||
| } | ||
|
|
||
| /** | ||
| * Stop the cache | ||
| * | ||
| * @param boolen | ||
| * @return string return contents of cache | ||
| */ | ||
| function end() | ||
| { | ||
| $data = ob_get_contents(); | ||
| ob_end_clean(); | ||
| $id = array_pop($this->nestedIds); | ||
| $group = array_pop($this->nestedGroups); | ||
| $this->save($data, $id, $group); | ||
| return $data; | ||
| } | ||
|
|
||
| } |
Oops, something went wrong.
a8ac90cThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing our custom error handlers fixed the situation in PHP 7. Are we certain we need them?
a8ac90cThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@onli @ophian Removing the error handler should only be a short-termin thing to get PHP7 runnig at all. Maybe you can add a PHP7 version check around that.
The proper solution would be to make our error handler recognized @ silenced error messages, not remove it. Also, our error handler in the future should make sure to print a full stack trace, currently there are cutoffs I believe.
a8ac90cThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@garvinhicking Can you shortly outline what is the goal of the error handler? For me it seems like its main purpose is to echo the error message only if production != true, but that's something the display_error setting is doing on its own. What is missing from php's default?
a8ac90cThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I said already: Pear packages are sort of dead nowadays.
Even when updated (like the cache lite files), they forgot to set the constructors in a compatible way for upper PHP versions, which is essentialy strangely enough.
http://board.s9y.org/viewtopic.php?f=2&t=20562&p=10444870&hilit=PHP7#p10444870
Onli, I never understand these too quick coding shots you are doing. While you don't seem to need them, you don't seem to care about their existence and just delete things like this. You have done that before even with things that are still open. I have to admit, this makes me angry.
Apart from that, the only way doing this, is using a PHP7 version check around these parts, like Garvin suggested.
And this exception error thing is a quite complicated thing too, since you need to handle different options by PHP itself, the ini and server sets, errors thrown in code, etc. (ie., @ temporarily changes the error_reporting state). You need to take your time to fully understand what they are doing in which situation, before cutting things off.
Yes Garvin that is true, but it differs for production false and debug, which is what we wanted to have.
a8ac90cThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't need to descend into the craziness that some of our code is (sometimes simply stemming from the craziness that PHP can be) to remove things that seems to be not needed anymore. We use git, we can restore everything. Here is a special situation: The error handler never worked properly, I'm very happy to see it go and get readable error messages – and am fully willing to restore the functionality we actually need as soon as someone is capable to clearly describe what its purpose is.
The new version from https://pear.php.net/package/Cache_Lite/ seemed to be working, but it is possible I just missed it failing as well. In that case we'd still need to update its constructor. pear/Cache_Lite#5 looks like it.
a8ac90cThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this "@Silencer" experience is more a matter of 2.1-dev === production false and a missing error_reporting(~E_WARNING) I assume, while "@silence-errors" still happen and now with PHP7 are thrown. And this in combination with the removal of E_STRICT Notices, which have been changed to other types.
a8ac90cThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ophian Currently, our code doesn't work with PHP7. If we temporarily need to strip error reporting to get our "bleeding edge" to work and continue finding other issues, that is an approach fine with me. It's a valid concern of @onlie being raised to check what exactly we need this custom error handling for.
That is something we can easily discuss without creating personal attacks or escalating the issue above what the commit is solely about. Please try to find a productive tone so that we can all work on this together.
If you see a good way to make the error reporting PHP7 compatible, please feel free to commit a better fix.
Now as for the actual reasons we have a custom error handler: First it is meant so that our alpha/beta users and developers can always report issues, and are not blocked by possible error_reporting/display_errors rules of their servers. Second, I believe we use that one to silently discard some PHP notices or warnings which otherwise uncaught would abort the whole codeflow. This is especially helpful to have our same codebase working on PHP 5.4 but also more recent versions.
One thing is certain: We cannot leave the error handler as it currently is, because PHP will in the future throw even more errors and delegate to our error handler, so we need to revise our system to properly work in all scenarios.
I am on vacation the next 10 days, so I'll not be able to contribute to this any more until then.
a8ac90cThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@garvinhicking
This is fine with me too, but use
and not delete it in total! I did not say anything else.
Absolutely. YES. But we don't want to raise the Serendipity min PHP requirement up to 7+ only!
I know several people using the dev repro in public, since they trust us not coding errors when not in real need, or at least fix them immediately. One of them is R.L. :)
I personally will start fixing PHP7 issues deeply when I have a working and stable test environment for it.
a8ac90cThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@garvinhicking Thanks for the explanation. I'll read up on current recommendations for this aspect. Though it seems to be working fine so far without the custom handler on at least uberspace – if we can actually react properly to those warning directly at code usage (like with session_regenerate_id) that might be an alternative better approach.
I actually tested s9y 2.1 with these changes on PHP 7 and PHP 5.6. Seemed to work fine.
I don't care. It is an alpha and stays an alpha regardless of usage. Besides, it is not as this immediately breaks anything.