Simplify SQL and translation pipeline #1
Comments
|
I like the sound of this:
Does this mean that civicrm-setup could be used to power the Drupal 8 installer, allowing us to remove the lines from the process that copy the SQL files? Ie., this line: https://gist.github.com/dsnopek/56311dbea347874e75180883efabb620#file-installing-civicrm-on-drupal-8-via-composer-sh-L54 That would be amazing! |
@dsnopek - Spot-on for both counts:
|
|
I have submitted a PR #9 in this direction, although my changes aren't complete and marked with WIP. In that PR I have added a new event listener |
For civicrm/civicrm-setup#1, this change allows `civicrm-setup` to read the XML metadata in the same way as GenCode -- which helps us to install the current schema without any intermediate files.
For civicrm/civicrm-setup#1, the general goal is to allow installing the database schema without needing to run `GenCode`. The current draft is crashing because the SQL does translation using `ts()`. But if you try to use `ts()` in a pre-boot environment, it will attempt to boot automatically so that it can read `$config->customTranslateFunction. This is a chicken-egg situation. We haven't yet reached the phase where the installer can boot up Civi... because we don't have the SQL... but the SQL can't be generated (translated) because Civi hasn't been booted. The aim of this patch is to loosen the coupling between `ts()` and `CRM_Core_Config` so that `ts()` can be used on its own. > Aside: You might ask how this works today -- basically, `GenCode` does a > database-less-boot, which placates `ts()`. However, the `civicrm-setup` > will eventually need to do full-boot, and AFAIK we don't have any > situations where one transitions from database-less-boot to full-boot; I > have a gut fear that such a transition would be its own slipper slope.
Overview
--------
The `ts()`/`{ts}` feature from `CRM_Core_I18n` has an option to escape output
(e.g. `{ts escape="sql"}Activities{/ts}`). However, SQL encoding is subjective
(depending on the connection details). For `{ts}` to support this feature, it
must have a dependency on the DB subsystem (e.g. it eventually calls `CRM_Core_DAO`).
For civicrm/civicrm-setup#1, we have an issue
where expressions like `{ts escape="sql"}Activities{/ts}` fail during
installation because `CRM_Core_DAO` is not fully available.
This change loosens the coupling, so that we can use `{ts escape="sql"}Activities{/ts}`
without needing `CRM_Core_DAO` per se.
Before
------
`ts`/`CRM_Core_I18n` is tightly coupled to `CRM_Core_DAO`. There is no way
to use `{ts escape=sql}Activities{/ts}` without spinning-up `CRM_Core_DAO`.
After
-----
`ts`/`CRM_Core_I18n` *defaults* to calling `CRM_Core_DAO`. However, this can
be overriden by manipulating a property.
Comments
--------
* I feel a little dirty keeping any coupling between i18n and the DB.
However, changing this would mean removing support for the
`{ts escape=sql}` option, and that would be a clear compatibility-break.
* Arguably, there may be a microsecond-level penalty in using
`call_user_func($SQL_ESCAPER)` rather than a specific class/function.
However, it's only incurred if you actually call `{ts escape=sql}`
while setting `$SQL_ESCAPER`, and that's pretty rare circumstance.
The typical runtime use-cases for `ts()` are unaffected.
The current version of
civicrm-setupstill relies on the same SQL files as the old installer -- which means:setup.shandregen.sh.en_US.To simplify the workflow:
plugins/installDatabase/InstallSchema.civi-setup.php...CRM/Core/CodeGen/*,xml/schema/*, andl10n/*to produce the SQL on-demand.Eventually, this should allow us to remove
regen.shand remove the variouscivicrm_data.xx_XX.mysqlfiles.The text was updated successfully, but these errors were encountered: