(WIP) Exploratory branch for composer/D8 builds #16328
Conversation
|
(Standard links)
|
accb7ff to
f6b8d42
Compare
Overview -------- The `$civicrm_paths` variable allows a sysadmin to override various path and URL computations. ```php $civicrm_paths['civicrm.packages']['url'] = 'https://example.com/libraries/civicrm/packages'; ``` The variable was originally tested with absolute URLs, and the subsequent examples/docs use absolute URLs (https://docs.civicrm.org/dev/en/latest/framework/filesystem/). These values are used to generate addresses, as in: ```php $abs = Civi::paths()->getUrl('[civicrm.packages]/foo.js', 'absolute'); $rel = Civi::paths()->getUrl('[civicrm.packages]/foo.js', 'relative'); ``` The patch allows more values in `$civicrm_paths` while ensuring that `getUrl()` works as expected. Before ------ The `getUrl()` requests only behave correctly if the override is an absolute URL - not if it's relative. After ----- The `getUrl()` requests behave correctly if the override is either an absolute URL or a relative URL. ```php $civicrm_paths['civicrm.packages']['url'] = 'https://example.com/libraries/civicrm/packages'; $civicrm_paths['civicrm.packages']['url'] = '/libraries/civicrm/packages'; ``` Comments -------- * `toAbsoluteUrl()` needs a base to prepend. I initially used `HTTP_HOST` but switched to `cms.root`, but correctly inferring scheme and host and port and httpd prefixes would be more complex - esp for background/CLI jobs. Using `cms.root` as the base is simpler. * It's tempting to allow recursive variables. But it's not actually needed for my purposes, and it would add complexity/maintenance. If it's really needed, one could update `toAbsoluteUrl()` to quickly check for variables (`$url[0] === '['`) and then evaluate them. But for now... I think the simpler format is fine.
1. The expected value of `$civicrmBaseUrl` should not be blank. The old value was a fiction of the unit-test environment. Be more realistic. 2. The expected values for extension-generated URLs should abide the same `$civicrmBaseUrl` as others.
Before
------
In Smarty, there is no way to request values from `Civi::paths()`.
After
-----
In Smarty, you can request values from `Civi::paths()` using these notations:
```
{crmResURL expr="[civicrm.root]/foo"}
{crmResPath expr="[civicrm.root]/foo"}
```
This will allow E2E matrix to send a signal about whether the default configurations produce valid URLs.
…es path This change should work as well as before, and it's more correct. However, it's not a full fix making the badge-editor work in D8 - the relevant code probably needs more of a rethink. But... this is cleaner...
Before
------
In certain local OSX+D8 test environments, when running the full suite, I got a large number of warnings like this:
```
Warning: simplexml_load_file(): I/O warning : failed to load external entity "/Users/totten/bknix/build/drupal8-clean/web/vendor/civicrm/civicrm-core//CRM/Campaign/xml/Menu/Campaign.xml" in /Users/totten/bknix/build/drupal8-clean/web/vendor/civicrm/civicrm-core/CRM/Core/Menu.php on line 99
```
After
-----
Those warnings seem to have gone away.
The menu still seems to populate correctly.
Comments
--------
This smells a lot like dev/core#742 -- e.g. it's the same warning and
(after adjusting for changes in line-numbering over the months) same line of
code.
Overview -------- This fixes an issue in which the git-based Civi-D8 sites have a malfunctioning menu -- because the asset URLs are malformed. Consider that the default value of ``imageUploadURL` is `[civicrm.files]/persist/contribute/`. Now, suppose you have one of these two configurations: ```php // Trailing slash configuration $civicrm_paths['civicrm.files']['url'] = 'http://tmpd8-clean.bknix:8001/sites/default/files/civicrm/'; $civicrm_paths['civicrm.files']['path'] = '/Users/totten/bknix/build/tmpd8-clean/web/sites/default/files/civicrm/'; // Trimmed slash configuration $civicrm_paths['civicrm.files']['url'] = 'http://tmpd8-clean.bknix:8001/sites/default/files/civicrm'; $civicrm_paths['civicrm.files']['path'] = '/Users/totten/bknix/build/tmpd8-clean/web/sites/default/files/civicrm'; ``` You could inspect to see if the URL is generated correctly by running these commands: ``` $ cv api setting.get return=imageUploadURL $ cv url -c imageUploadURL $ cv url -d "[civicrm.files]/persist/contribute/" $ cv path -c imageUploadDir $ cv path -d "[civicrm.files]/persist/contribute/" ``` Before ------ For generating paths (`cv path ...` or `Civi::paths()->getPath(...)`), it works to use either trailing-slash or trimmed-slash. For generating URLs (`cv url ...` or `Civi::paths()->getUrl(...)`), only trailing-slash cfg works. The trimmed-slash cfg leads to a bad URL with a missing slash (`.../default/files/civicrmpersist/...`). ``` [bknix-max:~/bknix/build/tmpd8prj-clean/web] cv api setting.get return=imageUploadURL { "is_error": 0, "version": 3, "count": 1, "id": 1, "values": { "1": { "imageUploadURL": "[civicrm.files]/persist/contribute/" } } } [bknix-max:~/bknix/build/tmpd8prj-clean/web] cv url -c imageUploadURL "http://tmpd8prj-clean.bknix:8001/sites/default/files/civicrmpersist/contribute" [bknix-max:~/bknix/build/tmpd8prj-clean/web] cv url -d "[civicrm.files]/persist/contribute/" "http://tmpd8prj-clean.bknix:8001/sites/default/files/civicrmpersist/contribute/" ``` This is surprising because the path expression (`[civicrm.files]/persist/contribute`) includes a slash... but it disppears in the final computation. After ----- Both configurations work, for paths and for URLs.
…iles Overview -------- This is a refactoring to allow consumers of `CRM_Core_CodeGen_Schema` to request the SQL without creating files. Before ------ Calls to `CRM_Core_CodeGen_Schema::generateDropSql` (ad nauseum) create SQL files. After ----- Calls to `CRM_Core_CodeGen_Schema::generateDropSql` (ad nauseum) return SQL strings. Comments -------- To ensure that files are still generated consistently, you can use this procedure: ``` git checkout MY_BASE_BRANCH ./bin/setup.sh -g cp -r sql sql.orig git checkout MY_MODIFIED_BRANCH rm sql/civicrm.mysql sql/civicrm_acl*mysql sql/civicrm_data*mysql sql/civicrm_drop.mysql sql/civicrm_navigation.mysql -f ./bin/setup.sh -g colordiff -ru sql.orig sql ```
4212f7f to
2fc2311
Compare
|
Ooh this will be mana from heaven. Let me know if you want some testing on some issues. I have an issue only with 5.21 (last I tried was 5.18 before that), where APIv4 explorer or ajax endpoint would throw an exception about permissions or something, even with uid:1 superuser .. and couldn't get it to work. I wonder if something in here that can help that. I hadn't been hardcoding global $civicrm_paths['civicrm_paths']['civicrm.packages'] .. now I wonder if I should have been. Is there an existing gitlab issue the changes in CRM_Core_Classloader are for? I've also had some success and a few issues running Drupal 8.8's Core PHPUnit Functional Javascript (Webdriver) tests with CiviCRM. I got it all basically working but there's also an issue installing the APIv4 extension programmatically in a test, and Civi being able to find the APIv4 classes (this is 5.13). I do programmatically clear caches enough to get all other extensions I've tried work, like Mosaico, CiviDiscount, etc.. But everything else works seems to work so far which is awesome .. Drupal 8 Core test infrastructure is an option for doing CiviCRM tests, at least Chromedriver / Webdriver tests. I had to some special stuff in setUp() .. like update civicrm.settings.php and put in settings overrides for global $civicrm_paths, and patch civicrm.install in the D8 module a tad. For each test it installs CiviCRM fresh. Could be totally unrelated to any of the commits you're working on for either issue IDK. Our way of installing with composer is to start with an ESR version tarball, remove the ./vendor and ./drupal directories .. and then committing that to a private repo, create tag, and alter where composer pulls civicrm/civicrm-core from in the Drupal 8.8 repo root composer.json "repositories" section. I'd patch our repo's version of the core files and build a fresh site. If there's a different way you want tested that would help, I'm game for that too. |
When running early-stage installation helpers like
`Civi\Test::headless()->...->apply()` or `Civi\Test:data()->...->populate()`
it can be hard to call `CRM_CodeGen_*` functionality - because that relies
on `ts()` which relies on other system services.
This change basically suspends/swaps some of the default `ts()` behavior
while the test system performs installation.
The change is in two sub-parts:
* Implement `asPreInstall()` (Files: `Civi/Test.php` and `CRM/Core/I18n.php`)
* Add the helper `\Civi\Test::asPreInstall()`
* Skip the use of "word replacements" during the pre-install phase
* Use `asPreInstall()` (Files: `Civi/Test/Data.php` and `Civi/Test/CiviEnvBuilder.php`)
* Wrap existing code with the helper `\Civi\Test::asPreInstall(). These changes look big, but that's
just whitespace -- inserting `asPreInstall()` changes the indentation.
This lays groundwork for another commit which will replace some references to `sql/civicrm*.mysql`
with references to `CRM_CodeGen_*`.
Before
------
If you have a copy of the `git` codebase and wish to run any of the headless
test suites, you *must* run `setup.sh` aka `GenCode` beforehand.
This is because certain files - `sql/civicrm.mysql` and
`sql/civicrm_data.mysql` - are used in the headless suite. As large
auto-generated files, they are not provided in git.
After
-----
The files `sql/civicrm.mysql` and `sql/civicrm_data.mysql` are no longer
required by the test suite -- the headless tests will directly use the
`GenCode` classes to produce the needful.
Comments
--------
The general thrust of this commit is to find spots which read an SQL file, e.g.
```php
\Civi\Test::execute(file_get_contents("foobar.mysql"));
```
and instead call the generator for that file, e.g.:
```php
$schema = new \CRM_Core_CodeGen_Schema(\Civi\Test::codeGen());
$result = $schema->generateFooBar();
\Civi\Test::execute($result['foobar.mysql']);
});
```
In doing so, we incorporate a couple tricks:
* The SQL content is cached for the duration of the test-run (`Civi\Test::$statics`).
Generating the SQL is not super expensive... but `Civi\Test::headless()->...apply()`
may be called thousands of times, so it could add-up. Just cache it.
* Most of the `generateFooBar()` functions depend in some fashion on `ts()`. This
commit depends on a preceding commit to make `ts()` more amenable to execution
during early stages of the test.
* Related discussion: https://chat.civicrm.org/civicrm/pl/ehohytqkf7bd5prg9w75dq4qqw
Gitlab issue - no. This round of "exploration" is stabilizing, though, so I'll split the changes into some smaller PRs so that there can be more description/discussion.
Right, let me try to comment on when one would or would-not care about using that:
Nice to hear! D8's test framework is a bit out of my purview right now (e.g. I'm working to get Civi's E2E/API/headless test suites to run on D8), but doing tests which invoke multiple frameworks can be fiddly, so I totally relate. |
|
So I just want to describe a bit about the disposition of this exploratory branch and its sibling civicrm/civicrm-buildkit#496 Basically, I've been sprinting with a view towards supporting D8 on our CI, which basically means that commands like this work: civibuild create mysite --type drupal8-clean --civi-ver master
civi-test-run -b mysite -j /tmp/junit phpunit-api4 phpunit-e2e
For the The test suites aren't running perfectly - there are failures. But I think it's cogent enough to make CI test sites and local dev sites from Civi's git repos - and run most tests on them. So I think it's time to close out this "exploratory branch" - splitting the patches into smaller PRs for review and filing issues on known/identified problems. |
|
@totten - per comment in chat I think the above is useful information to people following progress on this & it should be 'somewhere' - probably a gitlab. However, closing this PR now as it has been otherwise dealt with |
Overview
This is an exploratory branch with patches needed for (a) building Civi-D8 from git+composer, (b) running GenCode, (c) running unit tests. It is used by civicrm/civicrm-buildkit#496, and I'm opening as a PR so that we see test-runs and have Github generate diff files.
I expect that parts will eventually be broken out as smaller PRs for
master. The branch is based on5.21because I need to be able to try out builds on a couple different versions (b/c the QA processes will need to be able to test multiple versions).As an exploratory branch, it may be substantively changed without notice (commits added/rearranged/squashed/removed/rebased).