voryx / Thruway

PHP Client and Router Library for Autobahn and WAMP (Web Application Messaging Protocol) for Real-Time Application Messaging
MIT License
676 stars 117 forks source link

PHP 8 compatibility #351

Closed renepardon closed 3 years ago

renepardon commented 3 years ago

Hello,

to make the default example work, the Logger class at client/src/Logging/Logger.php needs to be adjusted to prevent errors like Required parameter $message follows optional parameter $object. I would suggest to either change the order or provide default parameters. e.g. $message = '' and on Line 31 for example:

public static function log($object = null, $level = LogLevel::INFO, $message = '', $context = [])

But even better:

public static function log($message, $object = null, $level = LogLevel::INFO, $context = [])

Right now it looks like this, which doesnt work with PHP 8:

public static function log($object = null, $level, $message, $context = [])

talisto commented 3 years ago

I'm stuck on this too, this library is the last remaining blocker I have in migrating a large application to PHP 8.

It appears that every call to this logger sends an object as the first parameter, so it seems like the easiest solution would be to simply make the $object parameter a required parameter, like so:

public static function log($object, $level, $message, $context = [])

Every method in that class would need to be changed.

This is actually an issue with the client library, which is a separate repository. Someone has already submitted a pull request with a fix for the issue, but that repository hasn't been updated since 2018, so I think the chances of the fix getting merged in are pretty slim..

https://github.com/thruway/client/pull/9

davidwdan commented 3 years ago

This should be fixed with https://github.com/thruway/client/releases/tag/0.5.5 Let me know if there are any other PHP 8 compatibility issues and I'll reopen this issue.

talisto commented 3 years ago

That's great, thank you!! Can you push the new version to packagist so that we can pull it in with composer? It looks like it isn't set to auto-update on packagist.