Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check for required PHP extensions #508

Merged
merged 3 commits into from Apr 4, 2019

Conversation

Projects
None yet
3 participants
@xh3n1
Copy link
Member

commented Apr 2, 2019

xh3n1 added some commits Apr 2, 2019

Added check for required PHP extensions by phpList
Signed-off-by: Xheni Myrtaj <myrtajxheni@gmail.com>
spacing
Signed-off-by: Xheni Myrtaj <myrtajxheni@gmail.com>
}
}
if (count($notinstalled) > 0) {
$message = "The following PHP extensions are missing:" . '<br>';

This comment has been minimized.

Copy link
@samtuke

samtuke Apr 2, 2019

Contributor

Should this be translatable?

This comment has been minimized.

Copy link
@xh3n1

xh3n1 Apr 2, 2019

Author Member

It cannot be translated as it stands above required language files.

@@ -23,6 +23,42 @@
// Remove when php5.X is unsupported, currently 31 Dec 2018, https://secure.php.net/supported-versions.php
require_once dirname(__FILE__).'/inc/random_compat/random.php';
// Check if required extensions are installed.
$phpExtensions = array(

This comment has been minimized.

Copy link
@samtuke

samtuke Apr 2, 2019

Contributor

How about naming this $requiredExtensions for clarity?

This comment has been minimized.

Copy link
@xh3n1

xh3n1 Apr 2, 2019

Author Member
Better variable name
Signed-off-by: Xheni Myrtaj <myrtajxheni@gmail.com>
@samtuke

samtuke approved these changes Apr 2, 2019

@xh3n1 xh3n1 merged commit 513d17f into master Apr 4, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@xh3n1 xh3n1 deleted the php-extensions branch Apr 4, 2019

@bramley

This comment has been minimized.

Copy link
Contributor

commented Apr 8, 2019

@michield @samtuke Despite what the documentation might say, some of the php extensions are not mandatory because the code sometimes checks before trying to use it.

Examples are curl and mbstring, see index.php

if (!function_exists('mb_strtolower')) {
    function mb_strtolower($string)
    {
        return strtolower($string);
    }
}

When this change is included in a release, if someone who does not have all of these extensions upgrades then phplist is going to stop working for them. I think that any change to making extensions mandatory needs a bit more thought.

Also, an earlier change added the file checkprerequisites.php explicitly for this kind of validation.

@xh3n1

This comment has been minimized.

Copy link
Member Author

commented Apr 12, 2019

@bramley
There are not always checks for functions before trying to use them, which leads to blank pages etc.
Following your comment here, I created this PR #513
where I moved the code to checkprerequisites.php, and removed some extensions that I think are more optional( those that have if-else step around functions).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.