-
Notifications
You must be signed in to change notification settings - Fork 5
Simplify SQL and translation pipeline changes #9
Conversation
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, |
@dsnopek good point. Well these generated files are stored in respective directories e.g. SQL files in 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. |
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. |
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 I also like to bring @totten in this discussion :) |
@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 ( Meta@monishdeb @dsnopek It sounds like you're both trying to negotiate a different approach which uses My perspective (as someone who was in some of those previous rounds) is different: the status-quo is a collection of kludges because we were too scared or ignorant to fix underlying problems. The project-structure here (with the separate installer library, deployed on new turf) provides a rare opportunity where we can address some underlying problems without risking a major break. So let's take it! General problemThe 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:
|
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:
|
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. |
da7d348
to
34dc61c
Compare
Thanks a lot @totten . I have now updated the PR and addressed all the 4 points. Here's what I have done against each:
Done. It needs me to do:
Done
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 :) |
src/Setup/DbUtil.php
Outdated
} | ||
|
||
public function generateSample($sqlPath) { | ||
$temporarySQLFilePath = implode(DIRECTORY_SEPARATOR, \Civi\Setup\FileUtil::createTempDir(), 'civicrm_generated.mysql'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this have []
s?
src/Setup/Template.php
Outdated
* @param string $outpath | ||
* Full path to the desired output file. | ||
*/ | ||
public function getConcatContent($inputs, $tempOutputFilePath) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I'm not following something, but... can we just remove all the $tempOutputFilePath
and file_*_contents()
stuff?
Basically:
$buffer = '';
foreach ($inputs as $infile) {
$buffer .= $this->smarty->fetch($infile) . "\n";
}
return $buffer;
Those changes generally sound appropriate. 👍 While trying it out, I made a few related changes in civicrm/civicrm-core#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 { |
src/Setup/Model.php
Outdated
* @property array $tables | ||
* List of CiviCRM tables extracted from xml schema files. | ||
* Values must only be array. | ||
* Ex: ['civicrm_contact', 'civicrm_group']. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think putting some schema metadata in Model
makes in terms of dataflows, although I wouldn't want downstream installer code (like cv core:install
or D8 civicrm.install
) to be aware of those parts of the model.
FWIW, if we wanted this to be private, it could go in something like $model->extras['schema.tables']
@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:
|
@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. |
@monishdeb I've posted some more commits on https://github.com/totten/civicrm-setup/commits/monishdeb-issue-1 . In combination with civicrm/civicrm-core#11677 and civicrm/civicrm-core#11682, it's able to get the basic, empty tables created. |
@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:
Working on this fix. |
Yup, I that's why I get too. :) My gut sense is that either 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 |
Yup sure, I am already working on it and thanks for sharing https://gist.github.com/totten/c2ab52dee8c8659944c9697616009f98 |
@totten @monishdeb I would really like to separate the i18n from this process as per #9 (comment) 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. |
@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 |
@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. |
@totten I have resolved the remaining issues. Here's what I have done:
Here's the latest result:
|
1. Typo in variable name ($fileType) 2. Add `assign()`, because a few other spots seem to expect it 3. Remove unused param ($outputath)
The main issue here is that `schema.tpl` is getting the wrong data. It was getting the DB name ```php generateCreateSql($model->srcPath, $model->db['database'], $model->tables) ``` But it should actually the full, parsed schema specification: ``` generateCreateSql($model->srcPath, $spec->database, $spec->tables) ``` So basically... `InstallSchema` needs access to the full `$specification` (which was only available in `AvailableTables`). And I don't currently see a use-case that requires exposing the schema as part of the installer model, so I think it's simpler to merge `AvailableTables` into `InstallSchema`.
@totten in this last commit 4f0315a I have fixed
I have rebuilt the DB snapshots and here's the diff:
|
And here's the zip which contains the newly created DB snapshots |
This version number winds up in the database (`civicrm_domain.version`); it needs to be the fully formed number so that future upgrades work correctly.
The behavior (prior to this commit) is not equivalent to published behavior because the normal/minimalist installation includes a smattering of sample records (e.g. sample case-types). This revision helps us do the old behavior by distinguishing between: * `generateBasicData()` is just the baseline data (countries, option-groups, etc). This is equivalent to the old behavior. * `generateSampleData()` includes the baseline data as well as some hypothetical/org-specific data (case-types, etc). This is similar in spirit to `civicrm_generated.mysql`, but it's not complete.
There are a bunch of `*.tpl` files which are incorporated into the baseline dataset. Most of these are listed in an array in `generateBasicData()`, but `civicrm_navigation.tpl` was treated differently (with its own `generateNavigation()`). This cuts down several SLOC. It appears to be equivalent based on reading and based on testing DB snapshots (before/after).
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 There were a few issues that came up, and I've pushed up patches for these:
|
Recap of QA steps that are passing:
So I think we can merge this. 🎉 |
This is awesome :) Thanks @totten and agree with the two patch. Just had a thought that as now the |
#1