PHP7 Compatibility

[garvinhicking]

Two issues so far:

1.) Constructors. serendipity_plugin::serendipity_plugin() is no longer allowed, we need to use __construct() for that. Check other instances, make sure that no call to ->constructor() or ::constructor() is called (because the function name will then no longer be available). That also applies to external classes (POP3fetcher, PEAR, ...). Check if we can write a parser that greps for (class XXX && function XXX) definitions.

2.) ErrorExceptions. We currently have instances where file_get_contents and session_regenerate_id() fails. Even if we surround a try/catch around it, our ExceptionHandler is executed and halts. So we need to make sure that @ silenced function called do NOT triger our exception handler. We'll need to see if that is possible at all. Else, we need to write our own serendipity_file_get_contents() handler which checks for "file_exists()" before.

[onli]

Okay, status update: The PEAR packages seem to be basically fine, apart from HTTP_Request. Version 2 is current and compatible, but the version 1 which we still use had a last update in 2008. To get rid of it, a number of plugins need an update, including core plugins like spartacus. My plan to keep this in spirit with a non-mayor update is to keep the file as it is, so we don't break external plugins we don't know about, but to update all occurrences we can control and to move them over to HTTP/Request2.

Onyx will be a bundled lib we'll either have to patch manually or replace with another lib: it uses HTTP/Request, but is not under development (even vanished from the web), and our version is customized anyway.

[garvinhicking]

Carrying on the discussion that was added to a8ac90c here:

I have now re-introduced our custom error handler, and slimmed it down. Also, in debug environments, the full stacktrace is shown, plus a note about sensitive data is shown. I also tried to add better/more code comments on what the error handler does.

The actual problem that hit us for PHP7 was that we always used ini_get('error_reporting') in our exception handler. This would never properly catch on @ silenced calls, for that we need to use error_reporting() to get a "0" so we can bailout.

Long story short - the current implementation allows me to log in to my s9y backend and frontendin PHP 7, also thanks to many of the other PHP7 compat changes so far. YAY! Great work :-)

@onli @ophian - Please check out the current code and see if everybody can live with this. If not, I'm open to suggestions and changes.

[onli]

Hi Garvin, nice to see that you tackled this.

I played with s9y on PHP 7 the last days and I saw no need for the error reporting code. It worked just fine without. To just set what is reported when s9y is set to production mode via

if ($serendipity['production'] === 'debug') {
    error_reporting(E_ALL ^ E_NOTICE); // is 32759 with 5.4+
}
if ($serendipity['production'] === false) {
    error_reporting(E_ALL & ~(E_NOTICE|E_STRICT)); // is 30711 with 5.4+
}

in serendipity_config.inc.php seemed sufficient to me. I might miss something of course (like: Did I see proper callbacks? I don't know anymore), but I'd be very happy if you evaluate (maybe again) with that perspective in mind whether the error handler is needed. Let's not keep it just because we had it.

Long story short - the current implementation allows me to log in to my s9y backend and frontendin PHP 7, also thanks to many of the other PHP7 compat changes so far. YAY! Great work :-)

Thanks :) There are still some places where Http_Request is used instead of Http_Request2, and I think Cache_Lite needs a constructor change as well (I do not understand why I do not see its deprecation message, but there is a PR in pear/Cache_Lite#5 that is not yet merged, which we should do manually if upstream does not do that soon).

[garvinhicking]

The first is, we want users to be able to set a "debug" mode with stacktraces and output, so that we can ask for that on forums and other support channels.

Then we possibly want to suppress some specific errors (like the "incompatible method declaration") so that older plugins can run without raising errors.

The current implementation is somewhat lighter and IMO better to grasp, and I do think that it helps in our debugging and support mechanisms, especially if we'll need to add some more specific PHP5 vs. PHP7 error checks we stumble opon in the next weeks.

[onli]

The first is, we want users to be able to set a "debug" mode with stacktraces and output, so that we can ask for that on forums and other support channels.

Sure. But we do get stacktraces already. Have a look at http://onli.columba.uberspace.de/s9y_dev/, I have 2.0 with PHP 7 and without the error handler running there. I added this error to the index.php:

echo "test"->is_array();

You'll see a proper error message and stacktrace in the browser. If I set $serendipity['production'] = true the error gets hidden.

I'll remove the error after you responded :)

Then we possibly want to suppress some specific errors (like the "incompatible method declaration") so that older plugins can run without raising errors.

That might be why we need it. I'd prefer to just fix the issues instead of suppressing he warnings, I think that's too risky. In any case, we should not have the error handler because we might possibly suppress warnings, but because we do suppress warnings.

Less code is good, and we already had problems with this for a long time. Of course, if we need it to get s9y to behave as we want on both PHP 5.5, 5.6 and PHP 7, then we just have to add it. But if not it would be cool to have one moving part less.

[garvinhicking]

How does the stacktrace look in more complex stacks?

Keeping the current handler and expanding on it in the future has the ulpside that we can already gather problems/issues with it, instead of doing it hastily in case we need it. Also that one signature thing alone is IMO worth having it... Also we could add plugin api hooks to that and possible doing better error catching for logging purposes... We've had the code for some time now and fixed issues that came, so I would feel sorry for all the effort on a basic foundation going away.

But yeah, this is not a clean decission. Your viewpoint is understandable, of course.

@mattsches @ophian - wanna chime in with simple reasons to keep or remove this?!

On 09.05.2016, at 17:46, onli notifications@github.com wrote:

The first is, we want users to be able to set a "debug" mode with stacktraces and output, so that we can ask for that on forums and other support channels.

Sure. But we do get stacktraces already. Have a look at http://onli.columba.uberspace.de/s9y_dev/, I have 2.0 with PHP 7 and without the error handler running there. I added this error to the index.php:

echo "test"->is_array();
You'll see a proper error message and stacktrace in the browser. If I set $serendipity['production'] = true the error gets hidden.

I'll remove the error after you responded :)

Then we possibly want to suppress some specific errors (like the "incompatible method declaration") so that older plugins can run without raising errors.

That might be why we need it. I'd prefer to just fix the issues instead of suppressing he warnings, I think that's too risky. In any case, we should not have the error handler because we might possibly suppress warnings, but because we do suppress warnings.

Less code is good, and we already had problems with this for a long time. Of course, if we need it to get s9y to behave as we want on both PHP 5.5, 5.6 and PHP 7, then we just have to add it. But if not it would be cool to have one moving part less.

—
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub

[onli]

How does the stacktrace look in more complex stacks?

I moved it into two function calls. Have a look. Looks fine to me.

Also that one signature thing alone is IMO worth having it...

What do you mean exactly?

[onli]

The move of the core to Http/Request2 is finished

[garvinhicking]

It might be nice to control the HTML pre/post output for the stacktrace, which the current approach offers... about the "signature thing", I mean that we are able to catch plugin troubles where the event_hook() declaration is not the same like event_hook() in the plugin API, and that we could add additional checks (like for the "invalid constructor name") there as well...

[onli]

While stuff like that could be nice, I still think we should not add another foundation for other stuff which might be useful later. If we don't use it now, all experience says we will never.

[onli]

I applied nacc's PR now manually to Cache_Lite. While testing that I also fixed the constructor in the cachesimple plugin (also tested that the shortcircuit still gets called).

I am not aware of any further work that needs to be done for PHP 7 :)

All that is left is to decide whether the error handler stays or goes. Since I at least now saw it working and giving a stacktrace where without there would be none, I still think it should go but can better accept if you decide otherwise.

[garvinhicking]

I found a cool tool "php7cc" which can be installed through composer and checks s9y code.

There are a couple of instances of =& assigned, a few foreach() loops with references, some hits on the Smarty library, but those left also don't look critical to me...

Let's wait a few more days to see if we get feedback from @mattsches or @ophian to give us a tipping point whether to keep or remove the handler ;)

[onli]

There are a couple of instances of =& assigned, a few foreach() loops with references, some hits on the Smarty library, but those left also don't look critical to me...

Will you go through them? Those might include the kind of behaviour changes I was worried about – things breaking without an error message.

[onli]

There is one additional incompatible bundled lib I missed, because it is not used in the core: XML/RPC. There is a successor package, https://pear.php.net/package/XML_RPC2, which is marked as unmaintained but saw a new release 4 days ago. We need to either:

1. update the old package,
2. move to the new one and update the plugins,
3. or remove this lib and the plugins that use this.

The plugins that use this are serendipity_event_weblogping and serendipity_event_ljupdate. I don't know the latter, but the concept of weblogpings is dead and I'd vote to remove that plugin. Is ljupdate still useful? If not, I think we could remove this lib.

[garvinhicking]

We'd definitely need the serendipity_event_xmlrpc plugin to work somehow
:) Maybe the current PEAR XML_RPC can also be updated easily somehow by
a few constructor fixes...

On 26.06.2016 02:43 , onli wrote:

There is one additional incompatible bundled lib I missed, because it is
not used in the core: XML/RPC. There is a successor package,
https://pear.php.net/package/XML_RPC2, which is marked as unmaintained
but saw a new release 4 days ago. We need to either:

1. update the old package,
2. move to the new one and update the plugins,
3. or remove this lib and the plugins that use this.

The plugins that use this are serendipity_event_weblogping and
serendipity_event_ljupdate. I don't know the latter, but the concept of
weblogpings is dead and I'd vote to remove that plugin. Is ljupdate
still useful? If not, I think we could remove this lib.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#399 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AAQrrsBdYEWWr-pSYZLsGpGqXdDvmSTaks5qPcsegaJpZM4IOI2S.

[onli]

Don't know why I missed that plugin. With that we really need to keep such a lib (though the plugin does not work with the wordpress app currently, see http://board.s9y.org/viewtopic.php?f=10&t=20743). To patch the old lib is an option, but I think we should move to something that is maintained. On the other hand, I'm completely not in the code – it would be better if someone else does the work and decides this.

[onli]

Just found this in the cache_lite issue: http://cweiske.de/tagebuch/php4-constructors-php7.htm

[onli]

The stack traces are not working properly. See https://board.s9y.org/viewtopic.php?p=10446654#p10446654. First it says:

For more details set $serendipity['production'] = 'debug' in serendipity_config_local.inc.php to receive a stack-trace.

Just to below print the error again, and to show a stack trace.

[garvinhicking]

We created a fatal error catcher, plus production variable should work.