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

Standalone - Add support for development-friendly file-layouts #26771

Merged
merged 23 commits into from Jul 16, 2023

Conversation

totten
Copy link
Member

@totten totten commented Jul 9, 2023

Overview

If you are doing development on civicrm-core.git and using the Standalone UF, then it's easier to checkout the git repo directly -- without creating a bespoke, top-level composer.json. (See: https://lab.civicrm.org/dev/core/-/issues/2998#note_92686)

This patch makes it possible to install the Standalone UF. I've tested with a total of 3 different layouts.

ping @artfulrobot

Technical Details

The essence of this update is to move a few files from civicrm-standalone.git (index.php, .htaccess, civicrm.config.php.standalone) to civicrm-core.git. Instead, there's a script (StandaloneScaffold.php aka ./tools/standalone/bin/scaffold) which will install these files.

This allows you to install the scaffolding in other structures. (One may use civicrm-standalone.git as a top-level project, but it is no longer required.)

Related PRs

Layout: HTTPD/Apache/Nginx - Composer (HAN-C; Existing)

Summary: This is the layout that's already/previously permitted.

Process: A site-builder sets up a vhost on Apache/Nginx or similar HTTPD. The site runs a top-level project (either a clone of civicrm-standalone.git or a new repo). The project uses composer to require key CiviCRM packages like civicrm-core, civicrm-packages, and civicrm-asset-plugin.

Example:

git clone https://github.com/civicrm/civicrm-standalone /var/www/example.com
cd /var/www/example.com
composer install
## Internally, this calls `StandaloneScaffold.php` to install `index.php`, `civicrm.config.php.standalone`, etc.

cv core:install --db=...
/var/www/example.com/civicrm.config.php.standalone
/var/www/example.com/data/civicrm.settings.php
/var/www/example.com/data/templates_c
/var/www/example.com/web
/var/www/example.com/web/index.php
/var/www/example.com/web/upload
/var/www/example.com/vendor/civicrm/civicrm-core
/var/www/example.com/vendor/civicrm/civicrm-packages

Trade-offs:

For production deployments (which merely consume civicrm-core), this has a few advantages:

  • You can keep the vendor/ tree outside of the web-root.
  • You can add/override/manipulate other packages via composer. (For example, you can get security updates for third-party dependencies without modifying civicrm-core.)

But for development deployments (which test+modify civicrm-core.git), it has a few disadvantages:

  • It's hard to update civircm-core:composer.json. (You can't simply run composer install.)
  • If you are switching revisions (e.g. to review patches or to bisect regressions), it's hard to get the right dependencies for that revision. (You can't simply run composer install.)
  • You live with the fear of blowing away the vendor/civicrm/civicrm-core folder when updating the top-level project.

Layout: HTTPD/Apache/Nginx - Direct (HAN-D; New)

Summary: This is a new layout. It flips the advantages/disadvantages above. It retains support for running on standard HTTPD's like Apache/Nginx.

Process: A developer sets up a vhost on Apache/Nginx or similar HTTPD. They clone civicrm-core.git into a public folder ./core.

Example:

mkdir -p /var/www/example.com/web
cd /var/www/example.com/web
git clone https://github.com/civicrm/civicrm-core core
git clone https://github.com/civicrm/civicrm-packages core/packages
cd core
composer install
./tools/standalone/bin/scaffold /var/www/example.com
## ^^ Install the scaffolding on the web-project

cv core:install --db=...
/var/www/example.com/civicrm.config.php.standalone
/var/www/example.com/data/civicrm.settings.php
/var/www/example.com/data/templates_c
/var/www/example.com/web
/var/www/example.com/web/index.php
/var/www/example.com/web/upload
/var/www/example.com/web/core
/var/www/example.com/web/core/packages

Layout: PHP Built-in Server (SRV; New)

Summary: This is a new layout. It drops the requirement to setup Apache/nginx/etc. Instead, you can treat civicrm-core as the main folder.

Process: A developer clones civicrm-core.git into a private folder. (For example, I often put development repos in ~/src/PROJECT-NAME.) Then one can launch PHP's built-in HTTP server.

Example:

git clone https://github.com/civicrm/civicrm-core ~/src/civicrm
git clone https://github.com/civicrm/civicrm-packages ~/src/civicrm/packages

cd ~/src/civicrm
composer install
./tools/standalone/bin/serve
## Internally, this calls:
## $ ./tools/standalone/bin/scaffold ./srv
## $ php -S localhost:8000 -t ./srv/web ./tools/standalone/router.php

cv core:install --db=...
~/src/civicrm
~/src/civicrm/packages
~/src/civicrm/srv
~/src/civicrm/srv/civicrm.config.php.standalone
~/src/civicrm/srv/data/civicrm.settings.php
~/src/civicrm/srv/data/templates_c
~/src/civicrm/srv/web
~/src/civicrm/srv/web/index.php
~/src/civicrm/srv/web/upload

Note that you don't need any particular HTTPD (Apache/Nginx) or PHP-FPM -- merely PHP-CLI. In lieu of a vhost, we create the folder ./srv with all the public+private data files that would otherwise appear in the vhost.

@civibot
Copy link

civibot bot commented Jul 9, 2023

Thank you for contributing to CiviCRM! ❤️ We will need to test and review the PR. 👷

Introduction for new contributors
  • If this is your first PR, an admin will greenlight automated testing with the command ok to test or add to whitelist.
  • A series of tests will automatically run. You can see the results at the bottom of this page (if there are any problems, it will include a link to see what went wrong).
  • A demo site will be built where anyone can try out a version of CiviCRM that includes your changes.
  • If this process needs to be repeated, an admin will issue the command test this please to rerun tests and build a new demo site.
  • Before this PR can be merged, it needs to be reviewed. Please keep in mind that reviewers are volunteers, and their response time can vary from a few hours to a few weeks depending on their availability and their knowledge of this particular part of CiviCRM.
  • A great way to speed up this process is to "trade reviews" with someone - find an open PR that you feel able to review, and leave a comment like "I'm reviewing this now, could you please review mine?" (include a link to yours). You don't have to wait for a response to get started (and you don't have to stop at one!) the more you review, the faster this process goes for everyone 😄
  • To ensure that you are credited properly in the final release notes, please add yourself to contributor-key.yml
  • For more information about contributing, see CONTRIBUTING.md.
Quick links for reviewers

@civibot civibot bot added the master label Jul 9, 2023
__FUNCTION__,
ts('In PHP 7.x, the built-in HTTP server cannot execute some security checks. This problem only affects local development on older versions of PHP.'),
ts('Incomplete Security Checks'),
\Psr\Log\LogLevel::ERROR,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this intended as level ERROR? It causes a problem in front-end tests that have php 7.4 as part of the matrix since it interprets it the same as any other red popup and so the test fails.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@demeritcowboy Sort of, but I'm ambivalent about the level - for the developer-user (foreseen audience of 5e8a6bb), one can imagine rationales for ERROR or WARNING or NOTICE. I've downgraded it to WARNING in hopes that it avoids knock-on effects in other suites.

Tangentially - Are you pointing out a general/likely issue (for any ERROR-level check) or an observed regression (with this specific check)? My observation was that several Security checks hung on php 7.4.29 cli-server (macOS; Civi-standalone). But if there's some other php74 where those tests work, then that's an interesting data-point.

Some security checks don't work on PHP 7.4's built-in HTTPD -- they cause
the system hang for >1min (*while internal HTTP calls timeout*). The
checks run whenever the admin logs in -- so having it hang is fairly
annoying (*for the developer who uses PHP built-in HTTPD*).

This only affects local development, so it's not critical to run the checks.

We'll skip the affected tests and show a simple (quick) error instead.
@demeritcowboy
Copy link
Contributor

Are you pointing out a general/likely issue (for any ERROR-level check) or an observed regression (with this specific check)?

It was with this specific check. Here's an example I ran earlier against cdntax with the earlier version of the PR applied, and it would be similar for webform tests: https://github.com/SemperIT/CiviCARROT/actions/runs/5501383178/jobs/10024925252#step:22:38. The giant inscrutable error message is saying "failed asserting no red alert popups", and if you were to download the html snapshots zip file for the run it shows them stopping on a page where the alert popup div is present.

My observation was that several Security checks hung on php 7.4.29 cli-server (macOS; Civi-standalone). But if there's some other php74 where those tests work, then that's an interesting data-point.

I can run it again against this version and see what happens. And actually that may explain one of the results in the earlier run where it seemed like WSOD, which I didn't bother looking into at the time.

@demeritcowboy
Copy link
Contributor

That passed. It's ubuntu 22.04.2, php 7.4.33, drupal 9.5.10.

@demeritcowboy
Copy link
Contributor

I just realized that as-is this PR would skip those checks, so the above didn't actually test them. I've run again with a modified version where it doesn't skip those checks. Still passes: https://github.com/SemperIT/CiviCARROT/actions/runs/5503275390/jobs/10028315079

@totten
Copy link
Member Author

totten commented Jul 10, 2023

@demeritcowboy OK, thanks. I'll keep that in mind as a point of reference - and when the various pieces are in place to try it more easily in more environments, I might try and see if my hang still appears in other environments. (But I think it's still good to proceed with skipping these checks for cli-server/php74.)

Incidentally, I said "hang", but it's a strong word -- the status-checks do eventually finish. It just takes >1 min each time. Interesting thing about those last two tests you linked (with and without HTTP-security checks) -- the "Run PHPUnit" step goes much faster without HTTP-security checks (3m vs 13m). Yet the outcome is the same (9 tests, 167 assertions). I don't know what timeframes are typical on that suite, but it could be that you've been experiencing the same issue as a drag on execution-time...

@demeritcowboy
Copy link
Contributor

Hmm, no the expected time is about 10-15 min, so 3m seems way off. I ran it again with the PR and it was also 3m. Hmm, tbd.

@mlutfy
Copy link
Member

mlutfy commented Jul 10, 2023

Are the issues with tests something introduced by this PR? (a blocker to merging) or something general that needs fixing later? (I have to admit I'm not sure I understand)

@demeritcowboy
Copy link
Contributor

The latest round no I don't think it's a blocker.

@mlutfy
Copy link
Member

mlutfy commented Jul 10, 2023

I would be tempted to merge this

@artfulrobot any objections?

@mlutfy mlutfy added the merge ready PR will be merged after a few days if there are no objections label Jul 10, 2023
@demeritcowboy
Copy link
Contributor

Commenting on the php7.4 built-in server slowness part - it happens in php 8.1 too, and it's the call to file_get_contents($url) in isBrowsable() which is trying to retrieve a zero-length index.html file. If I just put something in the file it's quicker, or if I switch it to guzzle. So I may do the guzzle as a followup and if that works out then the system check parts of this PR wouldn't be needed anymore.

@artfulrobot
Copy link
Contributor

@totten this sounds great. No objections from me. I've not tried it yet. And I see you can support HAN-D in buildkit too - nice.

This is a really good how to install, and why the options are like this documentation page. We probably need to make sure that it gets copied into somewhere suitable. Up until this point, I might have suggested civicrm-standalone.git/readme but now we don't need that repo...! Maybe we need a special standalone section in one of the main docs, e.g. installation guide?

@totten
Copy link
Member Author

totten commented Jul 16, 2023

(@demeritcowboy) Commenting on the php7.4 built-in server slowness part - it happens in php 8.1 too, and it's the call to file_get_contents($url) in isBrowsable() which is trying to retrieve a zero-length index.html file.

That sounds wild...

(@artfulrobot) Up until this point, I might have suggested civicrm-standalone.git/readme but now we don't need that repo...! Maybe we need a special standalone section in one of the main docs, e.g. installation guide?

Yeah, you're probably right - migrate to the installation guide (and also include suitable disclaimers about the experimental/WIP status of Standalone).

--

I did a bit more r-run on a different workstation. This basically still worked, but it did bubble up some other issues (e.g. compatibility with different flavors of PHP). But it doesn't change the utility of what's in here, and I don't think this stuff poses danger to any other use-cases. So I'll deal with new things in new PR.

@totten totten merged commit d5ac84d into civicrm:master Jul 16, 2023
3 checks passed
@totten totten deleted the master-s3 branch July 16, 2023 04:27
@artfulrobot
Copy link
Contributor

Thanks @totten. FWIW I tried for a couple of hours to r-run this on Friday but didn't manage it. I had been trying to run a patched buildkit to run the buildkit install from there but it was failing (couldn't find tools/scaffold). I'm sure this was caused by needing patched BK to bring in patched civi-SA to bring in patched core etc.

I'll retry now a few more of the key planks are merged, and hopefully get further.

@totten
Copy link
Member Author

totten commented Jul 17, 2023

(Aside: If anyone else has the same problem as ^^^, there's some parallel discussion here.)

@totten
Copy link
Member Author

totten commented Jul 18, 2023

(@artfulrobot) Up until this point, I might have suggested civicrm-standalone.git/readme but now we don't need that repo...! Maybe we need a special standalone section in one of the main docs, e.g. installation guide?

(@totten) Yeah, you're probably right - migrate to the installation guide (and also include suitable disclaimers about the experimental/WIP status of Standalone).

Here's a draft in the style of the other installation guides -- but with a big warning at the top. https://lab.civicrm.org/documentation/docs/installation/-/merge_requests/56

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
master merge ready PR will be merged after a few days if there are no objections
Projects
None yet
4 participants