tboothman / imdbphp

PHP library for retrieving film and tv information from IMDb
247 stars 84 forks source link

Monolog v2 #285

Closed jcvignoli closed 1 year ago

jcvignoli commented 1 year ago

I've seen a recet PR to improve compatibility with PHP 5.6. I would actually advocate the opposite.

Unless I'm mistaken, Imdbphp uses Monolog library due to its support of PHP5.6. It is dangerous to use Php5.6 in production environments. Why Imdbphp should still support it? The same goes as Monolog 1: it doesn't receive any update.

WordPress is a well known PHP library that still supports PHP5.6 I'm actually a WP plugin author using IMDBphp in my plugin. But WP can be hosted on webservers that are not modified in eons, old blogs and pages that people don't even remember they used to blog on it. WP has an automatic update that could destroy such websites. Imdbphp is a library utilises by developpers, quite a difference.

So I would like to advocate to drop PHP5.6 support. Coding has been changing a lot since PHP5.6. It's a safer, easier, more developper-oriented code that we could achieve if using... PHP8. Well, if that's sounds too ambitious, let's stick to php7.4, at least it receives security updates...

Php8 is 2 years old. Major companies haven't probably yet migrated to php8. But who are the users of imdphp? Major companies like banks? I guess not. Even if I would be curious to know who actually use IMDBphp...

I've been developping my WP plugin since Ozzi started the library (or was the guy before him?). 10 years ago. At that time, it was built on php5. Perhaps, a decade later, it's time to move on.

jreklund commented 1 year ago

imdbphp don't require any external libraries not native to PHP (unless you are an developer). imdbphp do have support for prs/log version 1 and prs/simple-cache version 1 and that's why you are getting Monolog 2 and not 3 in case you decide on that PSR-3 logger. We have support for all loggers, Monolog are just an example. Monolog 2 have support for PHP 7.2 and up.

I don't think we can just declare support for version 2 and 3 of prs/log. We have a local Logger and LoggerInterface that's compatible with version 1 in case you don't use Composer. If you do use Composer it will download it's own version, but I don't know if it uses our local interface or the one downloaded from Composer.

If our local interface gets replaced and you don't use an external logger imdbphp will crash. If it use an external library and our local interface get used it will crash as well. We can't do anything unless we require an external logger and cache or upgrade the projects PHP requirements.

Support for PSR-3 and PSR-16 where added to imdbphp 5 year ago and the standard itself where updated 14 Jul 2021 (log) and 29 Oct 2021 (simple-cache). Fairly new standard all things considered.

Regarding what version of PHP this project should support are another topic. PHP 7.4 will be dead next month and there are no official support for PHP 8.1 (in imdbphp) at the moment.

jcvignoli commented 1 year ago

I think you are correct in theory, it's up to the dev to select which psr lib to use. Your arguments do make sense. The thing is that we all use static tool analysers, code using the new options. PHP is safer and more secure in every release. So it becomes every day more complicated to code in older PHP versions. Anyway, that was just a suggestion. Thanks for taking the time to reply in such a detailed way!

tboothman commented 1 year ago

Not sure what to do with this really. We can use psr/log^3 but then would have to remove support for php <8. I think we've decided that supporting older versions is a good thing given who's using the library, as long as it's not making it too hard to support.

Would changing the required version of psr/log to ^1.0.1 || ^2.0 || ^3.0 help like they did in monolog? https://github.com/Seldaek/monolog/blame/f2f66cd480df5f165391ff9b6332700d467b25ac/composer.json#L17

The interface is the same for 1,2,3 but 2/3 have parameter and return types so technically the interfaces are all the same.

If I update to psr/log:^3 then php gets angry as the interfaces don't match:

 Declaration of Imdb\Logger::emergency($message, array $context = []) must be compatible with Psr\Log\LoggerInterface::emergency(Stringable|string $message, array $context = []): void in /mnt/c/Users/tom/code/imdbphp/src/Imdb/Logger.php on line 27
trovster commented 1 year ago

I have just come across this incompatibility when trying to use 8.0.0 and Laravel 10 which has just been released. The Laravel version requires monolog/monolog version 3.

tboothman commented 1 year ago

https://github.com/tboothman/imdbphp/pull/300 Does this help?

jcvignoli commented 1 year ago

This is perfect, thank you so much!

tboothman commented 1 year ago

🥳