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

CRM-21142 - CiviCRM installer - Check for XML module before install #10939

Merged
merged 2 commits into from Sep 4, 2017

Conversation

MegaphoneJon
Copy link
Contributor

@MegaphoneJon MegaphoneJon commented Sep 4, 2017

Overview

CiviCRM requires the PHP XML module. We should check for it as part of install requirements.

Before

CiviCRM installer will puke and leave an installation canary. See: https://civicrm.stackexchange.com/questions/20239/wordpress-civi-install-keeps-puking-with-canary-and-simplexml-load-file-erro

After

Installation is blocked by the lack of XML module.

Comments

I tested this on Ubuntu with a buildkit instance by renaming civicrm.settings.php, uninstalling the XML module (sudo apt remove php7.0-xml), restarting Apache, and running the installer. I reversed the changes when done.


@totten
Copy link
Member

totten commented Sep 4, 2017

Thanks, @MegaphoneJon. The test procedure looks good, and the code is clean/consistent with similar tests.

From a communications perspective... I think that PHP actually has several different XML extensions. Here's the list from a MAMP build of PHP 5.6:

$ php -r 'print_r(get_loaded_extensions());' | grep -i xml
    [3] => libxml
    [37] => SimpleXML
    [43] => xml
    [44] => xmlreader
    [45] => xmlwriter

In normal practice, maybe they're all bundled together, but there are some masochists who build their own PHP and tightly manage extensions. (ahem I'm looking at you, portage and arch.) They might read the error and enable xml or libxml... but the installer keeps saying that XML isn't available. Labeling it as "Simple XML" would make the dependency more precise.

@MegaphoneJon
Copy link
Contributor Author

Good point Tim. I relabeled it and did a Google search on the error to ensure that the first link would lead to a reasonable solution.

@eileenmcnaughton eileenmcnaughton merged commit 66841ae into civicrm:master Sep 4, 2017
@eileenmcnaughton
Copy link
Contributor

Thanks for the quick work on this!

@MegaphoneJon MegaphoneJon deleted the xml-install branch October 10, 2017 02:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants