search

LibriVox / librivox-catalog

LibriVox catalog and reader workflow application
https://librivox.org
MIT License
36 stars 17 forks source link

Upgrade to CodeIgniter 3 #7

Closed notartom closed 1 year ago

notartom commented 5 years ago

We're currently on CodeIgniter 2.1.1. In addition to the obvious, this prevents us from moving away from the EOL'ed PHP5.6 and upgrading to PHP7.

The upgrade guide is here: https://www.codeigniter.com/userguide3/installation/upgrade_300.html

kgroeneveld commented 3 years ago

There may very well be issues using CodeIgniter 2.1.1 with newer versions of PHP but so far on my dev setup I have not noticed an issue while using PHP 7.4.3.

notartom commented 3 years ago

Huh, I wonder where I got the CodeIgniter 3 == PHP 7 requirement from. Even their own doc says it should work with PHP5.6.

Out of curiosity, how did you set up your dev environment?

kgroeneveld commented 3 years ago

I setup my dev environment under kubuntu 20.04. I used the librivox-ansible scripts and hacked and slashed them until it worked. Such as changing all the php5.6 references to just php among other various things I probably should have made better notes on.

I think the bigger issue is does the old CodeIgnitor work properly with PHP 7, not whether CodeIgnitor 3 needs PHP 7. It would be nice to update the base operating system from ubuntu 16.04 to the latest LTS. Ubuntu 20.04 no longer has php5.6 (although there is probably some PPA for it or you could probably build it yourself).

Of course it would also be good to update CodeIgnitor eventually. CodeIgnitor 4 is out now so this issue should maybe be updated to be “Upgrade to CodeIgniter 4”.

It seems you have already done a bunch of work relating to the official CodeIgnitor 3 upgrade guide. I did want to eventually review/test that pull request in more detail but I am not sure when / if I will find the time. I am a LibriVox, CodeIgnitor and PHP newbie so it takes a lot of effort to review it properly.

There is likely some issue somewhere with using PHP 7 with the current LibriVox catalog. I have really only been testing search/browse and project launch templates so far. It could be horribly broken elsewhere. I didn’t intend to imply in my previous post that I think everything is OK just that it is not completely broken either.

I did have some issues relating to the newer mariadb in ubnuntu 20.04. I have been thinking of creating a new issue for that.

And it would be nice to update jquery, and basecamp, and, and … It’s a lot of work to keep everything up to date. And I do often think if it ain’t broke don’t fix it.

notartom commented 3 years ago

I setup my dev environment under kubuntu 20.04. I used the librivox-ansible scripts and hacked and slashed them until it worked. Such as changing all the php5.6 references to just php among other various things I probably should have made better notes on.

Feel free to contribute your changes back into master if you think they'd be useful to a wider audience (not that one exists, but...). I'm sure there's code rot in there, as those playbooks don't get run all that often...

I think the bigger issue is does the old CodeIgnitor work properly with PHP 7, not whether CodeIgnitor 3 needs PHP 7. It would be nice to update the base operating system from ubuntu 16.04 to the latest LTS. Ubuntu 20.04 no longer has php5.6 (although there is probably some PPA for it or you could probably build it yourself).

Of course it would also be good to update CodeIgnitor eventually. CodeIgnitor 4 is out now so this issue should maybe be updated to be “Upgrade to CodeIgniter 4”.

Based on their 3 to 4 upgrade doc, seems like that would involve a re-write of the catalog application, which I don't really want to undertake. I'm not sure for how much longer CI3 will remain maintained though... If I had unlimited time and energy I'd redo the whole thing in Python/Django since that's what I use for my day job (well, only Python, but I've played around with Flask for a professional side-project).

It seems you have already done a bunch of work relating to the official CodeIgnitor 3 upgrade guide. I did want to eventually review/test that pull request in more detail but I am not sure when / if I will find the time. I am a LibriVox, CodeIgnitor and PHP newbie so it takes a lot of effort to review it properly.

The upgrade patch series is WIP, and currently things are broken and need further patches to fix and/or finish the upgrade. I'm a CI and PHP newbie myself, and only know them from maintaining LibriVox over the years. You have the advantage of a wife who can explain the LibriVox bits, whereas I've never actually participated in the audiobooks side of the operation :)

There is likely some issue somewhere with using PHP 7 with the current LibriVox catalog. I have really only been testing search/browse and project launch templates so far. It could be horribly broken elsewhere. I didn’t intend to imply in my previous post that I think everything is OK just that it is not completely broken either.

I did have some issues relating to the newer mariadb in ubnuntu 20.04. I have been thinking of creating a new issue for that.

And it would be nice to update jquery, and basecamp, and, and … It’s a lot of work to keep everything up to date. And I do often think if it ain’t broke don’t fix it.

Yeah, you do feel like a bit of a rat on a treadmill with these things. I'm sure the people churning out CI and PHP versions and EOL'ing the old version do it for good reasons, but for a tiny volunteer-run operation like LibriVox, it becomes relentless. And it's not like we have cashflow to pay someone to do this for us as their day job.

kgroeneveld commented 3 years ago

Based on their 3 to 4 upgrade doc, seems like that would involve a re-write of the catalog application, which I don't really want to undertake.

For some reason I thought I read somewhere that CI 3 to 4 was an easy upgrade. But as the doc you link to points out that is not the case. I guess this issue should remain as "Upgrade to CodeIgnitor 3" then. That seems like a more attainable goal for now.

I'm sure the people churning out CI and PHP versions and EOL'ing the old version do it for good reasons ...

Just like the Python devs who made Python 3 incompatible with Python 2? And perl 6 was supposed to be basically a new language compared to perl 5 (until they decided to rename it)? Maybe we should re-write it in C! A decades only C program often still works with modern C compilers. (And yes, I am joking, in case anyone thinks I seriously think the LibriVox catlog should be written in C...)

kgroeneveld commented 3 years ago

I am still slowly working on trying to test the new CI3 branch on my dev system. I am now getting the following error:

A PHP Error was encountered Severity: Warning Message: include(/librivox/www/librivox.org/catalog/application/views/errors/html/error_exception.php): failed to open stream: No such file or directory Filename: core/Exceptions.php Line Number: 219

We have a bunch of error templates in the application/views/errors/html directory already but I suspect CI3 expects some more or different ones?

I added a var_dump(get_defined_vars()); after line 219 just to see what exception I was getting and I see ["message":protected]=> string(27) "Class 'Memcached' not found" in the big mess of output. So now to install memchced package and see what happens next.

notartom commented 3 years ago

Yep, looks like you've stumbled on https://github.com/notartom/librivox-ansible/commit/554db4675f8705e618228c158c68be5d42b3ae42

kgroeneveld commented 3 years ago

Yep, looks like you've stumbled on notartom/librivox-ansible@554db46

Yes, I knew about that commit and the memcached requirement before I got the error. My main concern was that we may be missing some error templates that CI3 expects.

Now that I have memcached installed the first time I tried to call up the workflow page I got:

Deprecated: __autoload() is deprecated, use spl_autoload_register() instead in /librivox/www/librivox.org/catalog/application/config/ci3/config.php on line 391 Deprecated: Methods with the same name as their class will not be constructors in a future version of PHP; Template has a deprecated constructor in /librivox/www/librivox.org/catalog/application/libraries/Template.php on line 33

I am not sure if that is a PHP 7 things or a CI3 thing. The weird thing is I cannot get the error to come up again even though I didn't change anything? I had something similar happen earlier. Is CI3 doing some caching or something that CI2 did not?

Anyway the basics are now running on my test system. I was planning on trying to review the code changes before doing much functional testing.

notartom commented 3 years ago

Deprecated: __autoload() is deprecated, use spl_autoload_register() instead in /librivox/www/librivox.org/catalog/application/config/ci3/config.php on line 391

That sounds like a PHP7 thing: https://www.php.net/manual/en/function.autoload.php

Deprecated: Methods with the same name as their class will not be constructors in a future version of PHP; Template has a deprecated constructor in /librivox/www/librivox.org/catalog/application/libraries/Template.php on line 33

Also a PHP7 thing:

Old-style constructors Prior to PHP 8.0.0, classes in the global namespace will interpret a method named the same as the class as an old-style constructor. That syntax is deprecated, and will result in an E_DEPRECATED error but still call that function as a constructor. If both construct() and a same-name method are defined, construct() will be called. In namespaced classes, or any class as of PHP 8.0.0, a method named the same as the class never has any special meaning. Always use __construct() in new code.

(from https://www.php.net/manual/en/language.oop5.decon.php)

twinkietoes-on commented 3 years ago

Since this change (I think, or shortly thereafter with one of the subsequent changes), we are not being logged out of the workflow until cookies are cleared. Is this a security issue that we would want to address, or is it okay? (EDIT to add: I accept cookies until I close my browser, so I'm not logged out unless I do that, which can be days or more where the workflow is accessible on my computer.)

notartom commented 3 years ago

It's configurable behaviour (ie, how long users remain logged in for). I don't think it really matters one way or another. In fact, one could argue that always typing in your password is more dangerous, keylogger-potential-wise, than just remaining logged in.

twinkietoes-on commented 3 years ago

Okay. Jo thought it was okay as it is now, but I wanted to get your opinion as well.

notartom commented 1 year ago

Filed https://github.com/LibriVox/librivox-catalog/issues/143 to track the login issue @twinkietoes-on reported 3 comments up from here, otherwise closing this since we're now running CI3.