search

anvc / scalar

Born-digital, open source, media-rich scholarly publishing that’s as easy as blogging.
Other
231 stars 73 forks source link

PHP 7 Compatibility #91

Closed ccc2lu closed 3 years ago

ccc2lu commented 6 years ago

I want to upgrade the server where I run Scalar to Debian Linux "Stretch". I tried installing Scalar on a test server running Stretch and found some issues with PHP 7, which is the version of PHP that comes with Stretch.

Here's what I've found so far:

In system/application/views/modules/dashboard/book_style.php, I had to remove a "continue;" on line 320 because that's no longer valid in PHP 7. "break" and "continue" are only allowed in loop or "switch" contexts, not in an "if" statement. I replaced the "continue" with a pair of empty curly braces, and an "else {" on the next line. I then scrolled down to the "endif;" on line 495 and inserted a "}" on the line above. I believe that does the same thing as the "continue" was doing.

In codeigniter.php, on line 13 it says "error_reporting(E_ALL);". There are a bunch of deprecation, notices, and warnings that show up in the browser as a result. The warnings should certainly be fixed. The deprecation messages make sense to be logged but shouldn't show up in the browser. Notices it makes sense to allow to be suppressed entirely. This is certainly a matter of opinion, but I think Scalar should respect the error reporting level set in php.ini. I spent a fair amount of time trying to figure out why the setting I put in there seemed to be ignored before I thought to look in Scalar's own code for calls to "error_reporting" that changed it.

ccc2lu commented 6 years ago

The other warning messages I see in the browser are related to only passing variables by reference: system/database/DB.php line 133 system/codeigniter/Common.php line 149

And about subclass method declarations not matching the parent class's method declaration: In system/application/models/user_model.php's get_all method In system/application/models/book_model.php's get_all method In system/application/models/book_model.php's slug_exists method

I added unused default parameters to the slug_exists method to silence the warning since it didn't affect anything. I didn't make changes to the get_all methods though. The parameters they were missing were first in the list. I'm not sure what just adding default parameters would do to them.

craigdietrich commented 6 years ago

Hey thanks for all of this!

Nb the items in 'system' (not 'system/application') are CodeIgniter that we didn't write ourselves. That said it will be a long time before we upgrade to CodeIgniter 3 so I think it's fine to go ahead and make the changes there.

I'll take a closer look at this when I can, asap!

dmer commented 4 years ago

Hello, I'm planning a local install of Scalar and was also hoping to be able to make use of PHP 7 - I saw this tweet from reclaimhosting about a patched version, but I'm not sure where I can find that in this repo? https://twitter.com/ReclaimHosting/status/1157066217905172493 Thanks! Derek

craigdietrich commented 4 years ago

@dmer Scalar is up to speed with PHP 7.2 as of Release v2.5.5. If you go to the Releases section you'll see that we're up to 2.5.7 so you'll be all set!

dmer commented 4 years ago

Great! Thanks @craigdietrich

dmer commented 4 years ago

Sorry I just realized I have one other related question, can you point me to where the required/supported versions for things like PHP and MySQL etc. are listed? Thanks!

craigdietrich commented 4 years ago

@dmer That documentation doesn't exist :)

Really, you should be fine. Scalar doesn't really have any dependancies and aside from the PHP 7.2 thing, doesn't really get impacted by versions.

Take a look at the top of INSTALL.txt, though, it lists a few considerations.