Tatoeba / tatoeba2

Tatoeba is a platform whose purpose is to create a collaborative and open dataset of sentences and their translations.
https://tatoeba.org
GNU Affero General Public License v3.0
714 stars 132 forks source link

CakePHP Upgrade to 4.x #3123

Open cblanken opened 7 months ago

cblanken commented 7 months ago

This isn't anywhere near complete, but I wanted to get the ball rolling on issue #3067 to upgrade to CakePHP 4.x since 3.x is EOL.

Progress

I've been following the CakePHP 4.0 Upgrade Guide. It has a few steps:

Abandoned packages

There are several abandoned packages still in use even after meeting the above dependency targets. It might be better to resolve these in another PR, but I'll document them here just in case.

Remaining Deprecation Warnings

The deprecations I listed seem to be the most prolific, but as far as I can tell I got all instances of each. There are few more though.

[Deprecated (16384)](javascript:void(0);): Router::parseNamedParams() is deprecated. 2.x backwards compatible named parameter support will be removed in 4.0 - /home/vagrant/Tatoeba/src/Controller/AppController.php, line: 136
 You can disable deprecation warnings by setting `Error.errorLevel` to `E_ALL & ~E_USER_DEPRECATED` in your config/app.php. [CORE/src/Core/functions.php, line 311]
[Deprecated (16384)](javascript:void(0);): ServerRequest::query() is deprecated. Use getQuery() or the PSR-7 getQueryParams() and withQueryParams() methods instead. - /home/vagrant/Tatoeba/src/Controller/SentencesController.php, line: 509
 You can disable deprecation warnings by setting `Error.errorLevel` to `E_ALL & ~E_USER_DEPRECATED` in your config/app.php. [CORE/src/Core/functions.php, line 311]
[Warning (512)](javascript:void(0);): Unable to emit headers. Headers sent in file=/home/vagrant/Tatoeba/vendor/cakephp/cakephp/src/Error/Debugger.php line=855 [CORE/src/Http/ResponseEmitter.php, line 53]
[Warning (2)](javascript:void(0);): Cannot modify header information - headers already sent by (output started at /home/vagrant/Tatoeba/vendor/cakephp/cakephp/src/Error/Debugger.php:855) [CORE/src/Http/ResponseEmitter.php, line 154]
[Warning (2)](javascript:void(0);): Cannot modify header information - headers already sent by (output started at /home/vagrant/Tatoeba/vendor/cakephp/cakephp/src/Error/Debugger.php:855) [CORE/src/Http/ResponseEmitter.php, line 183]
[Warning (2)](javascript:void(0);): Cannot modify header information - headers already sent by (output started at /home/vagrant/Tatoeba/vendor/cakephp/cakephp/src/Error/Debugger.php:855) [CORE/src/Http/ResponseEmitter.php, line 269]

The first one listed here in particular may be a bit troublesome. I'm not familiar with all the routing done for the application, but I assume named parameters are still in use for some routes, thus the invokation of parseNamedParams(). Unfortunately, it seems 4.0 and onward won't support them, so it may be necessary to write some custom middleware to handle whatever requests are still using named parameters.

This is still a work in progress, but I'll see what I can do about the remaining deprecation warnings to get the application ready to use the 4.0 upgrade tool.

jiru commented 7 months ago

What an ambitious PR! :open_mouth: Thank you for getting on with this task.

I assume named parameters are still in use for some routes

Named parameters used to be the default for CakePHP < 3. They were used on Tatoeba for every page before. For example, if you look at this old archived page, you can see the "next page" links have a /page:2 parameter (as opposed to ?page=2). CakePHP 3.x changed that default, we also did, but we did not want to break existing links, so just to keep backward compatibility we use parseNamedParams(). You can try opening https://tatoeba.org/fr/sentences_lists/show/724/page:2 and you’ll see it still brings you to page 2 (a redirect would have been better though :pensive:). That’s the only reason.

cblanken commented 7 months ago

(a redirect would have been better though 😔). That’s the only reason.

Do you think it'd be best to inline the parseNamedParams() function from CakePHP 3.x? Otherwise this might be a good opportunity to stop supporting named parameters. If someone with access to the server could check the logs, there may not even be any recent usage of links with named parameters. In that case it'd probably be best to remove completely since they won't be supported by CakePHP in the future.

jiru commented 6 months ago

As far as I know we only keep logs for a short time so they probably won't give a good overview of old links usage. To keep maintaining old links or not to and potentially break things, that is the question! Personally I'm in favor of keeping old links working as long as it's not a burden (which I don't think it is, I mean it's fine to keep our own copy of parseNamedParameters()). I don't think anybody likes getting 404 errors when browsing the web.

cblanken commented 1 month ago

Okay, I've done what I can with this, but I don't think I'm familiar enough with CakePHP to sort out what's remaining. I've documented the changes as well as I could with links to the appropriate migration guide pages.

Maybe I'll return to it once I've worked with the codebase a bit more, but for the time being, anyone else who wants to pick this up is welcome to it.

[!NOTE]
When advancing the application to 4.x with the upgrade tool, be aware that PHP 8.x is required to run the tool, even though 4.x applications only have a minimum requirement of PHP 7.2. For more details, see https://book.cakephp.org/4/en/appendices/4-0-upgrade-guide.html#upgrade-to-php-8-0.

jiru commented 1 month ago

Thank you very much for working on this, @cblanken!