Simplify SQL and translation pipeline changes

monishdeb commented 6 years ago

https://github.com/civicrm/civicrm-setup/issues/1

dsnopek commented 6 years ago

I took a peek at the code, but it's not super clear to me: where do these generated files get placed? The question I'm trying to answer is if the web user (when doing a web, as opposed to cli, install) will have permission to write the files? It's not uncommon for the web user to only have permission to write to a limited set of directories (for example, sites/default/files for public files on a Drupal site).

monishdeb commented 6 years ago

@dsnopek good point. Well these generated files are stored in respective directories e.g. SQL files in civicrm/sql/* and these are the set of tasks executed during schema generation https://github.com/civicrm/civicrm-core/blob/master/CRM/Core/CodeGen/Main.php#L112L120

For web-user I can't think of a alternative right now as those generated files must be placed inside civicrm directory as those will be required to configure and install Civi.

dsnopek commented 6 years ago

Hm. For Drupal 8, we could maybe have a composer script run the codegen? The user running composer would have to be able to write to those directories.

For other Drupal versions wanting to do a web install, though, we'd need to be able write them somewhere else (maybe even just the temporary directory?) and then somehow get CiviCRM to find them at an alternative location.

monishdeb commented 6 years ago

For other Drupal versions wanting to do a web install, though, we'd need to be able write them somewhere else (maybe even just the temporary directory?) and then somehow get CiviCRM to find them at an alternative location.

Earlier I refrained to put my comment in this direction because I thought that it would be too much work to use the underlying code of each Codegen task so that it can use the generated files in temporary directory of choice, for installation purpose but then on second thought, I think that will be possible as we just need to make each CRM_Core_CodeGen_Tasks::run($this) flexible enough to do so.

I also like to bring @totten in this discussion :)

totten commented 6 years ago

@monishdeb Kudos on getting into this code-structure. I know the patterns are a bit different from what we've seen in runtime Civi, so that's probably taken a bit of reading/experimenting.

Let me paraphrase b81ea0c: basically, it adds a new API function ($setup->generateSchema()) which is really a wrapper for calling GenCode. As you indicate, this is incomplete/different. Bear with me for a few minutes to dig in.

General problem

The basic, gut-level problem (boiled down to the minimum) is that installing Civi from source doesn't feel like installing other things from source.

As applied to SQL generation...

Try to think of another PHP application/framework which generates SQL the way Civi does -- i.e. as a standard practice, the first step after downloading the code is to call a big script that pre-generates 100+ SQL files. And the second step is to run some other process which picks 1 or 2 of them.

Does D7 do that? No. D8? No. WP? No. Joomla? No. Backdrop? No. Symfony? No. Laravel? No. In typical usage, all of these systems generate and execute the SQL on-demand (after the DB has become available). No intermediate files needed.

(This not saying "SQL files suck". There are good use-cases for them. drush may sync SQL files as part of the dev/prod workflow. Symfony has a CLI option to dump to a SQL file. That's all good, but it's optional, based on the admin's desire, and it's not the standard workflow. Making it the standard for all installation is the atypical thing which increases the mental-load.)

Put another way... #1 is saying Let's do the work of CRM_Core_CodeGen_Schema without GenCode.

I'd expect that writing a patch for #1 would go roughly like this:

Copy the code from CRM_Core_CodeGen_Schema
Put it in InstallSchema.civi-setup.php
Modify it so that it reads $e->getModel() instead of $this->config.
Modify it so that it executes the SQL instead of writing it to a temp file.

totten commented 6 years ago

Aside: my suggestion above was to copy+edit. This is OK by me since the original would eventually become irrelevant. Alternatively, it's also valid to take a more conservative tack and avoid any moment of duplication. Ex:

In civicrm-core, take most of the code from CRM_Core_CodeGen_Schema and move it in a helper/utility.
- The helper accepts the DB metadata as input; for output, it returns the SQL code (string).
- CRM_Core_CodeGen_Schema calls the helper to get SQL as string. Then it writes the SQL string(s) to file(s).
In civicrm-setup, InstallSchema.civi-setup.php calls the helper. to get SQL as a string. Then it executes the SQL string(s).

nganivet commented 6 years ago

This all goes in the right direction, kudos to all for working on this.

An issue with the current code is that the initial database population is dependent on a 'Primary language' as it populates a number of options/strings with their translated content. But when the 'Primary language' is later changed in the UI, those same options/strings are not 're-translated' into the new language.

Looking at the options you are now considering, I wonder if we could also split the database population into (a) schema generation, including all option/strings in English and (b) translation of options/strings into another language.

This way, we could call the (b) part when switching the Primary language in the admin UI.

Adding @mlutfy since this is related to i18n.

monishdeb commented 6 years ago

Thanks a lot @totten . I have now updated the PR and addressed all the 4 points. Here's what I have done against each:

Copy the code from CRM_Core_CodeGen_Schema

Put it in InstallSchema.civi-setup.php

Done. It needs me to do:

Add 3 DbUtil helper functions
Modified sourceSQL() to take both SQL content OR filename
Add plugins/init/AvailableTables.civi-setup.php to extract Civi table information to $model->tables as in core https://github.com/civicrm/civicrm-core/blob/master/CRM/Core/CodeGen/Specification.php#L32
Add 2 help function to FileUtil to create Directory.
Add template Wrapper src/Setup/Template.php
Add smarty Util wrappersrc/Setup/SmartyUtil.php

Modify it so that it reads $e->getModel() instead of $this->config.

Done

Modify it so that it executes the SQL instead of writing it to a temp file.

Done, made changes in InstallSchema.civi-setup.php which now generate and execute the SQL content. However, for generated data we need to fetch SQL content from multiple files, reason why I have used temp directory to store the content in a temp file and then return its content at one go. Here's an example

...
 \Civi\Setup\DbUtil::sourceSQL($model->db, \Civi\Setup\DbUtil::generateCreateSql($srcPath, $model->db['database'], $model->tables));
...
// where
function generateCreateSql($srcPath, $model->db['database'], $model->tables)) {
  ...
   return $template->getContent($srcPath . 'xml/templates/schema.tpl');
}

LAST and most important task remaining - testing :)

totten commented 6 years ago

Those changes generally sound appropriate. 👍 While trying it out, I made a few related changes in https://github.com/civicrm/civicrm-core/pull/11677 , and (so far) this small change was also useful:

diff --git a/plugins/init/AvailableTables.civi-setup.php b/plugins/init/AvailableTables.civi-setup.php
index 95484aa..0a4c5a5 100644
--- a/plugins/init/AvailableTables.civi-setup.php
+++ b/plugins/init/AvailableTables.civi-setup.php
@@ -22,7 +22,7 @@ if (!defined('CIVI_SETUP')) {
       $xmlBuilt = \CRM_Core_CodeGen_Util_Xml::parse($versionFile);
       $buildVersion = preg_replace('/^(\d{1,2}\.\d{1,2})\.(\d{1,2}|\w{4,7})$/i', '$1', $xmlBuilt->version_no);
       $specification = new \CRM_Core_CodeGen_Specification();
-      $specification->parse($schemaFile, $buildVersion);
+      $specification->parse($schemaFile, $buildVersion, FALSE);
       $tables = $specification->tables;
     }
     else {

totten commented 6 years ago

@monishdeb , I was trying to run locally and fixed a few issues in the process.

https://github.com/totten/civicrm-setup/commits/monishdeb-issue-1

but it now gets as far as:

$ ~/src/cv/bin/cv core:install --setup-path=$HOME/src/civicrm-setup  --cms-base-url=http://dmaster.l -f -vvv

...

Fatal error: Call to undefined function ts() in /Users/totten/buildkit/build/dmaster/sites/all/modules/civicrm/CRM/Core/Smarty/plugins/block.ts.php on line 57

Call Stack:
    0.0003     241320   1. {main}() /Users/totten/src/cv/bin/cv:0
    0.0090     902352   2. Civi\Cv\Application::main() /Users/totten/src/cv/bin/cv:27
    0.0458    3548888   3. Symfony\Component\Console\Application->run() /Users/totten/src/cv/src/Application.php:17
    0.0506    3911536   4. Symfony\Component\Console\Application->doRun() /Users/totten/src/cv/vendor/symfony/console/Application.php:124
    0.0509    3912592   5. Symfony\Component\Console\Application->doRunCommand() /Users/totten/src/cv/vendor/symfony/console/Application.php:193
    0.0510    3913168   6. Symfony\Component\Console\Command\Command->run() /Users/totten/src/cv/vendor/symfony/console/Application.php:850
    0.0519    3920152   7. Civi\Cv\Command\CoreInstallCommand->execute() /Users/totten/src/cv/vendor/symfony/console/Command/Command.php:257
    0.6581   29001832   8. Civi\Cv\Command\CoreInstallCommand->runSetup() /Users/totten/src/cv/src/Command/CoreInstallCommand.php:75
    0.6969   29365600   9. Civi\Setup->installDatabase() /Users/totten/src/cv/src/Command/CoreInstallCommand.php:179
    0.6989   29372712  10. Symfony\Component\EventDispatcher\EventDispatcher->dispatch() /Users/totten/src/civicrm-setup/src/Setup.php:204
    0.6989   29374144  11. Symfony\Component\EventDispatcher\EventDispatcher->doDispatch() /Users/totten/buildkit/build/dmaster/sites/all/modules/civicrm/vendor/symfony/event-dispatcher/Symfony/Component/EventDispatcher/EventDispatcher.php:53
    0.6992   29375520  12. call_user_func:{/Users/totten/buildkit/build/dmaster/sites/all/modules/civicrm/vendor/symfony/event-dispatcher/Symfony/Component/EventDispatcher/EventDispatcher.php:164}() /Users/totten/buildkit/build/dmaster/sites/all/modules/civicrm/vendor/symfony/event-dispatcher/Symfony/Component/EventDispatcher/EventDispatcher.php:164
    0.6992   29376216  13. Civi\Setup::{closure:/Users/totten/src/civicrm-setup/plugins/installDatabase/InstallSchema.civi-setup.php:44-74}() /Users/totten/buildkit/build/dmaster/sites/all/modules/civicrm/vendor/symfony/event-dispatcher/Symfony/Component/EventDispatcher/EventDispatcher.php:164
    0.7650   30440552  14. Civi\Setup\DbUtil::generateNavigation() /Users/totten/src/civicrm-setup/plugins/installDatabase/InstallSchema.civi-setup.php:55
    0.7658   30445736  15. Civi\Setup\Template->getContent() /Users/totten/src/civicrm-setup/src/Setup/DbUtil.php:259
    0.7658   30445736  16. Smarty->fetch() /Users/totten/src/civicrm-setup/src/Setup/Template.php:42
    0.9973   32681176  17. include('/private/var/folders/s5/w16pvsx11q54y87_g91xwf_80000gn/T/templates_cBRYG8I.d/%%3E^3E2^3E245C50%%civicrm_navigation.tpl.php') /Users/totten/buildkit/build/dmaster/sites/all/modules/civicrm/packages/Smarty/Smarty.class.php:1270
    0.9974   32813584  18. smarty_block_ts() /private/var/folders/s5/w16pvsx11q54y87_g91xwf_80000gn/T/templates_cBRYG8I.d/%%3E^3E2^3E245C50%%civicrm_navigation.tpl.php:48

monishdeb commented 6 years ago

@totten sorry about that, I should have tested on my local to fix them all after making changes :( Thanks a lot for fixing these issues. I added your commits in this PR.

totten commented 6 years ago

@monishdeb I've posted some more commits on https://github.com/totten/civicrm-setup/commits/monishdeb-issue-1 . In combination with https://github.com/civicrm/civicrm-core/pull/11677 and https://github.com/civicrm/civicrm-core/pull/11682, it's able to get the basic, empty tables created.

monishdeb commented 6 years ago

@totten great. I have applied both the core patches and after pulling your commits in this branch, when I executed the command it got stuck at:

cv core:install --settings-path=http://localhost:8888/test-civicrm/sites/default/civicrm.settings.php --src-path=/Users/monish/src/civicrm --cms-base-url=http://localhost:8888/test-civicrm/ --db=mysql://root:root@127.0.0.1:8889/test_civicrm --model=/Users/monish/www/civicrm-master/.core-install-settings.json 
Found code for civicrm-core in /Users/monish/src/civicrm
Found code for civicrm-setup in /Users/monish/src/civicrm/vendor/civicrm/civicrm-setup
Creating file http://localhost:8888/test-civicrm/sites/default/civicrm.settings.php
Creating civicrm_* database tables in test_civicrm

  [Civi\Setup\Exception\SqlException]                                                                                                                                           
  Cannot execute INSERT INTO civicrm_mail_settings (domain_id, name, is_default, domain) VALUES (@domainID, 'default', true, 'EXAMPLE.ORG'): Column 'domain_id' cannot be null

Working on this fix.

totten commented 6 years ago

Yup, I that's why I get too. :)

My gut sense is that either generateNavigation() isn't executing correctly -- e.g. maybe some inputs are wrong, or maybe there's another *.tpl that needs to be loaded first. Can you dig in on this error?

Aside: Yesterday, I tried to make a map of the various SQL+TPL files and the timing/ordering with which they traditionally loaded. May be useful: https://gist.github.com/totten/c2ab52dee8c8659944c9697616009f98

monishdeb commented 6 years ago

Yup sure, I am already working on it and thanks for sharing https://gist.github.com/totten/c2ab52dee8c8659944c9697616009f98

nganivet commented 6 years ago

@totten @monishdeb I would really like to separate the i18n from this process as per https://github.com/civicrm/civicrm-setup/pull/9#issuecomment-365701095 while we are working on this. Can you specify what would need to be done for this separation and I will do the work? IMO it's just removing translations from the .tpl files and adding this somewhere else, but I would need to know what the 'somewhere else' is - another .tpl file, a piece of PHP code, etc. Thanks. @mlutfy FYI and comments.

totten commented 6 years ago

@nganivet I agree it's a bit frustrating to change the language post-installation, and I see the appeal of re-translating, but I don't really see how coupling that problem to this one helps.

If the idea is to make more of these things translated in real-time, then that's going to be an across-the-board reworking.

If the idea is to update DB records post-install, then I think the algorithm would be the same with or without this patchset. It might look something like this pseudo-code:

function translateData() {
  $fixableColumns = [
    ['civicrm_location_type', 'display_name'],
    ['civicrm_option_value', 'label'],
    //...
  ];
  foreach ($fixableColumns as $fixMe) {
    list ($table, $column) = $fixMe;
    foreach (query("SELECT $column AS oldValue FROM $table") as $row) {
      $newValue = ts($row['value']);
      query("UPDATE $table SET $column = $newValue WHERE $column = $oldValue");
    }
  }
}

You'd have to read civicrm_data.tpl to make a list of tables/columns to fix-up. And it might take a few minutes. And it only works if the start-language is en_US (though that's fixable if you build a reverse-map of the *.po). But in any case, I don't really see why it needs to be tied into this PR/issue.

nganivet commented 6 years ago

@totten Thanks. I am discussing this change as part of this PR because I want to make sure that this new behavior (ie. always install in en_US and then translate by calling a PHP function) is compatible with the new 'SQL and translation pipeline' that you are building.

If it is, I will go ahead and work on this as part of a separate PR.

monishdeb commented 6 years ago

@totten I have resolved the remaining issues. Here's what I have done:

When we execute the SQL files in the given order, it throws Column 'domain_id' cannot be null error triggered from navaigation.tpl because this file is executed too early before populating the necessary data. So I have placed the SQL-run for navigation at the end i.e. AFTER generate-data-sql execution that is responsible to insert domain record and other necessary data.
After resolving the above, I got another set of issues where essential Civi constants (like CIVICRM_UF_DSN) were not defined. So when I looked at the sequence of plugins I found that civicrm setting file creation task is placed after installSchema/*. I think that's odd to include the uninstall events as part of civi setup. So I have defined the order in which the plugin files should be picked which fixes this issue. I then just need to include the civi setting file before the SQL run here

Here's the latest result:

$ cv core:install --settings-path=http://localhost:8888/test-civicrm/sites/default/civicrm.settings.php --src-path=/Users/monish/src/civicrm --cms-base-url=http://localhost:8888/test-civicrm/ --db=mysql://root:root@127.0.0.1:8889/test_civicrm --model=/Users/monish/www/civicrm-master/.core-install-settings.json 
Found code for civicrm-core in /Users/monish/src/civicrm
Found code for civicrm-setup in /Users/monish/src/civicrm/vendor/civicrm/civicrm-setup
Creating file http://localhost:8888/test-civicrm/sites/default/civicrm.settings.php
Creating civicrm_* database tables in test_civicrm

monishdeb commented 6 years ago

@totten I have addressed all the 3 issues you have pointed in my earlier fixes and here's the last commit -https://github.com/civicrm/civicrm-setup/pull/9/commits/8cb1d2451eef520103b0aa3bb118384114d08c9f

totten commented 6 years ago

OK, so I'm testing the latest patches -- and think I'm seeing the issue you were talking about wrt to CIVICRM_ constants. I could partially get around this by putting the system into the more paranoid "upgrade mode" (e.g. patch Setup::init() to say define('CIVICRM_UPGRADE_ACTIVE', 1);). But still had a problem...

And I think this problem is actually good/right -- because it's getting to the heart of the underlying issue, i.e. that the php-mysql integration wants a specific connection for encoding SQL, and GenCode.

One way to look at the problem is look at these two listeners:

| #2    | InstallSchemaPlugin->installDatabase()                                                               |
| #3    | closure(/Users/totten/src/civicrm-setup/plugins/installDatabase/BootstrapCivi.civi-setup.php@13)     |

No ordering of these two would currently work:

With the ordering of InstallSchema=>BootstrapCivi, the InstallSchema fails because it can't encode SQL because we haven't set CIVICRM_DSN because we haven't read civicrm.settings.php (which is part of the other step, BootstrapCivi)
With the ordering of BootstrapCivi=>InstallSchema, the BootstrapCivi would fail because it calls CRM_Core_Config::singleton() which tries to read info from DB tables which don't exist (because the other step, InstallSchema, handles creation).

I'm gonna take a quick stab at splitting BootstrapCivi into smaller stages, e.g.

| #2    | closure(/Users/totten/src/civicrm-setup/plugins/installDatabase/LoadSettings.civi-
| #3    | InstallSchemaPlugin->installDatabase()                                                               |
| #4    | closure(/Users/totten/src/civicrm-setup/plugins/installDatabase/LoadConfigObject.civi-

totten commented 6 years ago

OK, splitting BootstrapCivi into smaller stages didn't help. I got tired of tracing through CRM_Core_DAO to figure out how to make it initialize in the pre-installation environment. Instead, here's a patch which loosens the dependency:

For civicrm-core: https://github.com/civicrm/civicrm-core/pull/11699
For civicrm-setup: 625faccbd7b008bd45d44e949bc401d3ced2e5c3 and dcfa37dd1ed4232355522a5d32d95c63dc4520c4 (from https://github.com/totten/civicrm-setup/commits/monishdeb-issue-1)

monishdeb commented 6 years ago

Yup all the issues with - DSN error, civi contstant not found, escape sql were earlier resolved by ordering the plugins here, mainly to call InstallFiles/InstallSettingsFile.civi-setup.php before installDatabase/InstallSchema.civi-setup.php and then include the setting file just above hitting generate-SQL codes - https://github.com/civicrm/civicrm-setup/pull/9/files#diff-4a4f1bf220f5de60a738a7d9fbea7f77R83 . But then it's not right approach to do so!

monishdeb commented 6 years ago

@totten I have pulled your commits from branch monishdeb-issue-1 and rebased against master to pull the latest codes added for Drupal8 and here's the commit list - https://github.com/civicrm/civicrm-setup/pull/9/commits

Also, I have executed the installer on this branch and it WORKED like a charm :) I think this PR is ready for merge as all the issues been addressed.

totten commented 6 years ago

@monishdeb Agree that this PR is in a working state now -- a nice milestone. :)

However, I think there's still a couple things to do before InstallSchema is really independent of GenCode...

InstallSchema still reads some of the *.mysql files produced by GenCode (civicrm_data.{$seedLanguage}.mysql, civicrm_acl.{$seedLanguage}.mysql, civicrm_data.mysql, civicrm_acl.mysql). The SQL should be generated direct from *.tpl.

Once it seems to be working, we should do a final test to ensure that it actually produces consistent data. Basically, we'd have a list of configurations to spot-check (e.g. "Clean en_US database", "Clean fr_FR database", "Demo US database") -- then you could compare the databases produced by the old code and new code.

For example, suppose you have this script which creates a database for each configuration:

#!/bin/bash

## Install CiviCRM with various configurations. For each one, make a snapshot of the resulting database.

SITEROOT=$HOME/buildkit/build/dmaster
CV=$HOME/src/cv/bin/cv
DATADIR=/tmp/snapshots-$( date "+%Y-%m-%d-%H-%M-%S" )

echo "Snapshots will be written to $DATADIR"

cd "$SITEROOT"
mkdir -p "$DATADIR"

echo "Perform a clean install with default locale"
$CV core:install --setup-path=$HOME/src/civicrm-setup -f --cms-base-url=http://dmaster.l
amp sql:dump -Ncms --passthru="--skip-extended-insert --skip-dump-date" > "$DATADIR/en_US.mysql"

echo "Perform a clean install with fr_FR"
$CV core:install --setup-path=$HOME/src/civicrm-setup -f --cms-base-url=http://dmaster.l --lang=fr_FR
amp sql:dump -Ncms --passthru="--skip-extended-insert --skip-dump-date" > "$DATADIR/fr_FR.mysql"

echo "Perform a demo install with sample data"
$CV core:install --setup-path=$HOME/src/civicrm-setup -f --cms-base-url=http://dmaster.l -m loadGenerated=1
amp sql:dump -Ncms --passthru="--skip-extended-insert --skip-dump-date" > "$DATADIR/sampledata.mysql"

Then run the script once with the old code (excluding #9) and once with the new code (including #9). Compare the output...

colordiff -ru /tmp/snapshots-FIXME-OLD /tmp/snapshots-FIXME-NEW

and decide if the differences are meaningful (e.g. "we missed all the ACL data") or incidental (e.g. "these logs have different timestamps").

monishdeb commented 6 years ago

@totten thanks for the script :) I have now able to fix the installSchema plugin to carry and populate the DB on basis of seedLanguage provided (if any). Here's the last commit - https://github.com/civicrm/civicrm-setup/pull/9/commits/c2657cc118b5ea95e76957598835948264baf676

Now on colodiff between old vs new snapshot w/o this patch, here's the result:

DB instance on default locale install https://gist.github.com/monishdeb/63b1f4a1fd4a5c2b12904b2900f96c0d
DB instance on es_ES locale install https://gist.github.com/monishdeb/7c6770e8192b083cd2835acb21d0a6e4
DB instance on generatedSmaple install https://gist.github.com/monishdeb/bbb1196dbd37baa2c709fc9a2af7680c

So the last case, has some missing data w.r.t to old DB snapshot. working on it.

Here's the zip file of the old and new DB snapshots: tmp.zip

totten commented 6 years ago

@monishdeb so i've walked through each of the diffs, notes the diffs, and categorized them

snapshot: en_US
- these look different, but i don't think it matters
  - across the board, AUTO_INCREMENT's differ slightly
  - civicrm_contact: trivial timestamp difference. don't care.
  - status_override_end_date: this column seems different, but i think that's happenstance of the local dev/recent change
- these differences are surprising and meaningful. don't have a theory/explanation.
  - civicrm_currency: values gone missing
  - civicrm_domain: need to set version
- it seems like the new code is failing to perform the first-run setup steps (e.g. civi.core.install doesn't run, or it fails to trigger DatabaseInitializer, or that fails to do system.flush)
  - across the board, triggers are missing
  - civicrm_install_canary: missing in newer snapshot.
  - civicrm_menu: seems to be empty in the new ssnapshot
  - civicrm_setting: resCacheCode and installed haven't been set in new
- these differences all seem like symptoms that civicase data has been loaded into the new but not the old. i'm not sure whether the old or new snapshot is better.
  - civicrm_case_type: extra records in new
  - civicrm_group: extra records in new
  - civicrm_relationship_type: extra records in new
- these tables have large differences which are hard to analyze, but there's a fair chance they'll go away after fixing the above
  - civicrm_cache
  - civicrm_option_value
snapshot: es_ES
- this does appear to be translated, which is good
- all the discrepancies seem to mirror discrepancies in the en_US data
- conclusion: nothing to worry about
snapshot: sampledata
- NOTE: for brevity, I'm ignoring variations that match the en_US variations
- these tables all have significantly less data in the new variant, suggesting that we're not do anything like GenerateData
  - civicrm_activity civicrm_activity_contact civicrm_address civicrm_contact civicrm_contact_type civicrm_contribution
  - civicrm_contribution_page civicrm_contribution_soft civicrm_email civicrm_entity_financial_account civicrm_entity_financial_trxn
  - civicrm_entity_tag civicrm_event civicrm_financial_item civicrm_financial_trxn civicrm_group civicrm_group_contact
  - civicrm_line_item civicrm_loc_block civicrm_membership civicrm_membership_block civicrm_membership_log
  - civicrm_membership_payment civicrm_membership_type civicrm_msg_template civicrm_note civicrm_participant
  - civicrm_participant_payment civicrm_payment_processor civicrm_pcp civicrm_pcp_block civicrm_phone
  - civicrm_pledge civicrm_pledge_block civicrm_premiums civicrm_premiums_product civicrm_price_field
  - civicrm_price_field_value civicrm_price_set civicrm_product civicrm_relationship civicrm_subscription_history
  - `civicrm_tell_friend civicrm_uf_join civicrm_website
- these custom data tables are missing
  - civicrm_value_constituent_information_1
  - civicrm_value_donor_information_3
  - civicrm_value_food_preference_2
- uncategorized
  - civicrm_cache
  - civicrm_dashboard
  - civicrm_custom_field
  - civicrm_custom_group
  - civicrm_navigation
  - civicrm_option_group
  - civicrm_option_value

monishdeb commented 6 years ago

@totten in this last commit https://github.com/civicrm/civicrm-setup/pull/9/commits/4f0315a21d23b420a694bff17540df296ee0389a I have fixed

Load sample date using sql/civicrm_generated.mysql as I found that using regen.sh we not only rely on php script GenerateData to populate sample data but it also pulls out other data from respective files and that all being dumped in that mysql file. So I felt why not use that generate mysql file directly.
Now locale instance has civicrm_currency and civicrm_domain record
civicrm_install_canary is present
civicrm_menu is populated with new data.
In civicrm_setting these are the settings present:
Triggers are present

I have rebuilt the DB snapshots and here's the diff:

DB instance on default locale install https://gist.github.com/monishdeb/63b1f4a1fd4a5c2b12904b2900f96c0d
DB instance on es_ES locale install https://gist.github.com/monishdeb/7c6770e8192b083cd2835acb21d0a6e4
DB instance on generatedSample install https://gist.github.com/monishdeb/bbb1196dbd37baa2c709fc9a2af7680c

monishdeb commented 6 years ago

And here's the zip which contains the newly created DB snapshots tmp.zip

totten commented 6 years ago

Thanks for the udpates, @monishdeb! I pulled down the code and ran DB dumps, the diff's were definitely closer than before. 👍 And I agree that it's easier to use civicrm_generate.mysql (which is currently committed/distributed via all channels) than to try to reproduce regen.sh.

There were a few issues that came up, and I've pushed up patches for these:

In cv, there's a test case CoreLifecycleTest which installs and uninstalls using civicrm-setup. The test was failing for two reasons. (a) In the interim since it was developed, we've started flagging alphas/betas again -- and a version regex failed. (Patch: https://github.com/civicrm/cv/commit/8cd12e4bf5d59c71d96545f8af8faa3ded9a82de) (b) In the final data-set, civicrm_domain.version looked like 5.2 instead of the expected 5.2.beta1 -- which is significant for handling upgrades. (Patch: c3bed33)
There were still some unexpected data-differences -- on a clean (non-demo) installation, the old and new behavior differed on whether to include civicrm_sample.tpl and case_sample.tpl. I think the old behavior was more correct -- i.e. we shouldn't registered "Adult Day Care Referral" and "Housing Support" on a clean site -- those should only show up on a demo site. (Patch: 839da8f)

totten commented 6 years ago

Recap of QA steps that are passing:

Run CoreLifecycleTest. Passed on all tested environments (drupal-empty,wp-empty,backdrop-empty).
Run the bash script from https://github.com/civicrm/civicrm-setup/pull/9#issuecomment-367856417 before and after. Compare the results. I ignored cache tables. The differences were few and trivial (e.g. different timestamps on the 1 contact record).
Delete all locally-generated *.mysql files (based on grep sql .gitignore). Manually run cv core:install (both in basic/clean mode and with loadGenerated=1) and browse through the data in the UI. The site appeared to work.
Manually run cv core:install (as above). Then use set-version.php and cv upgrade:db to simulate a DB upgrade. The civicrm_domain.version appeared to progress nicely.

So I think we can merge this. 🎉

monishdeb commented 6 years ago

This is awesome :) Thanks @totten and agree with the two patch. Just had a thought that as now the civicrm-version.php is merged to core so we can use its utility function \_CiviVersion_\Util::findVersion() to fetch version from xml. But that doesn't matter as I'll introduce this minor change in later PR.

civicrm / civicrm-setup

Simplify SQL and translation pipeline changes #9

Meta

General problem

As applied to SQL generation...