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

dev/drupal#193 Replace custom installer with Civi\Setup #679

Open
wants to merge 2 commits into
base: 7.x-master
Choose a base branch
from

Conversation

mlutfy
Copy link
Member

@mlutfy mlutfy commented Feb 7, 2024

https://lab.civicrm.org/dev/drupal/-/issues/193

Replaces the custom CiviCRM installer by the better-supported \Civi\Setup.

This only impacts people still using drush8 (ex: Aegir).

Copy link

civibot bot commented Feb 7, 2024

(Standard links)

@civibot civibot bot added the 7.x-master label Feb 7, 2024
Copy link

civibot bot commented Feb 7, 2024

The issue associated with the Pull Request can be viewed at https://lab.civicrm.org/dev/drupal/-/issues/193

@mlutfy
Copy link
Member Author

mlutfy commented Feb 7, 2024

Interesting fail:

E2E\Core\LocalizedDataTest::testLocalizedData The German SQL should match the German pattern.
Failed asserting that false is true.

so I guess we'll be able to see if this PR helps for that fail: civicrm/civicrm-core#29311

* Don't need to setup a web-page controller (`$ctrl`)
* Fix inheritance of default`server` when it has an alternate port
* Options chosen via drush-cli should override autodetected options
    * This is the most substantive part of the change. I had trouble installing to an alternate DB.
      The reason is that `init()` actually kicks of auto-detection of things like DB.
      If the installer-client wants to set its own value of `$model->db`, then it's best
      to do after `init()`.
@totten
Copy link
Member

totten commented Feb 9, 2024

@mlutfy Very nice!

I did some r-run with the patch and had a few small issues. Pushed an update for those.

For my testing, I used these commands:

## Use second database.  Leave `lang` empty.
drush civicrm-install --dbhost=127.0.0.1:3307 --dbname=dmastercivi_3r007 --dbuser=dmasterciv_4i5or --dbpass=XXX \
        --tarfile=/dev/null -l http://dmaster.127.0.0.1.nip.io:8001 -vv

## Use second database. Set locale to en_US.
drush civicrm-install --dbhost=127.0.0.1:3307 --dbname=dmastercivi_3r007 --dbuser=dmasterciv_4i5or --dbpass=XXX \
        --tarfile=/dev/null -l http://dmaster.127.0.0.1.nip.io:8001 -vv --lang=en_US --langtarfile=/dev/null

## Use second database.  Set locale to fr_FR.
drush civicrm-install --dbhost=127.0.0.1:3307 --dbname=dmastercivi_3r007 --dbuser=dmasterciv_4i5or --dbpass=XXX \
        --tarfile=/dev/null -l http://dmaster.127.0.0.1.nip.io:8001 -vv --lang=fr_FR --langtarfile=/dev/null

## Use single database. Set locale to de_DE.
drush civicrm-install --tarfile=/dev/null -l http://dmaster.127.0.0.1.nip.io:8001 -vv --lang=de_DE --langtarfile=/dev/null

## Use single database.  Leave `lang` empty.
drush civicrm-install --tarfile=/dev/null -l http://dmaster.127.0.0.1.nip.io:8001 -vv 

With the patch, they're all working for me. (I didn't test anything that requires downloading a tar file -- but it looks like that stuff wasn't touched by the patch.)

Note: The reported failure (testLocalizedData) is pre-existing/unrelated (https://lab.civicrm.org/dev/translation/-/issues/84) and doesn't need to block this PR. If it's working for you, and if the revisions are OK, then +1 for merging.

@mlutfy
Copy link
Member Author

mlutfy commented Feb 19, 2024

jenkins, test this please

@mlutfy
Copy link
Member Author

mlutfy commented Feb 19, 2024

@totten thanks for the testing & fix!

That translation bug is on my radar too. I don't think it's been fixed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants