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

xml/GenCode.php - Fix execution in default locale (without l10n data) #16583

Merged
merged 1 commit into from
Feb 18, 2020

Conversation

totten
Copy link
Member

@totten totten commented Feb 18, 2020

Overview

This fixes a recent regression in which xml/GenCode.php fails to execute in certain configurations.

Initial report: civicrm/civicrm-buildkit#503

This most likely stems from https://lab.civicrm.org/dev/translation/issues/30 aka #15408 which was first merged into 5.23.

Before

  • If the folder l10n exists in its traditional location, and if you run ./bin/setup.sh -g, then it correctly executes.
  • If the folder l10n does not exist in its traditional location, and if you run ./bin/setup.sh -g, then it fails with an error like this:
    + php -d mysql.default_host=127.0.0.1:3306 -d mysql.default_user=dmasterciv_abcde -d mysql.default_password=abcd1234 GenCode.php schema/Schema.xml '' Drupal
    
    civicrm_domain.version := 5.23.beta1
    
    Parsing schema description schema/Schema.xml
    Extracting database information
    Extracting table information
    PHP Fatal error:  Uncaught RuntimeException: Invalid configuration: the [cms.root] path must be an absolute URL in /home/me/.../Civi/Core/Paths.php:174
    Stack trace:
    #0 /home/me/.../Civi/Core/Paths.php(151): Civi\Core\Paths->toAbsoluteUrl('/', 'cms.root')
    #1 /home/me/.../Civi/Core/Paths.php(176): Civi\Core\Paths->getVariable('cms.root', 'url')
    #2 /home/me/.../Civi/Core/Paths.php(151): Civi\Core\Paths->toAbsoluteUrl('/', 'civicrm.root')
    #3 /home/me/.../Civi/Core/Paths.php(224): Civi\Core\Paths->getVariable('civicrm.root', 'path')
    #4 /home/me/.../Civi/Core/Paths.php(84): Civi\Core\Paths->getPath('l10n')
    #5 [internal function]: Civi\Core\Paths->Civi\Core\{closure}()
    #6 /home/jo in /home/me/.../Civi/Core/Paths.php on line 174
    

After

You can run ./bin/setup.sh -g with or without the l10n folder.

When running GenCode, the CMS URL is set to a fake value http://gencode.example.com/do-not-use.

Comments

There's an argument to be made that this shouldn't be necessary: when running GenCode, it should only create PHP files, and none of the output should depend on the CMS URL - because that's liable to change when you deploy the PHP code. If someone did try to generate URL at such an early stage, it's arguably good to generate an error. In point of fact, GenCode is not actually using the CMS URL.

The oddity stems from the contract of CRM_Utils_System_* (specifically, getCiviSourceStorage() and getDefaultFileStorage()) which do double-duty providing both path and URL. To avoid duplicate work, Civi\Core\Paths uses the same grain of information - it tracks the pairs of path+URL. And because it works at the same grain, we get into logic like:

  • If you want the civicrm.root path, then you need to also get the civicrm.root URL.
  • If you want the civicrm.root URL, then you need to get the cms.root URL.

A more aggressive fix might be to split getCiviSourceStorage() and getDefaultFileStorage() so that it's possible to get the path and URL separately; then revise Civi\Core\Paths to take advantage of the finer-grained contract. However, splitting those things would be a more invasive patch, and we're currently in RC.

Overview
--------

This fixes a recent regression in which `xml/GenCode.php` fails to execute in certain configurations.

Initial report: civicrm/civicrm-buildkit#503

Before
------

* If the folder `l10n` exists in its traditional location, and if you run `./bin/setup.sh -g`, then it
  correctly executes.
* If the folder `l10n` does not exist in its traditional locacation, and if you run `./bin/setup.sh -g`,
  then it fails with an error like this:
  ```
  + php -d mysql.default_host=127.0.0.1:3306 -d mysql.default_user=dmasterciv_abcde -d mysql.default_password=abcd1234 GenCode.php schema/Schema.xml '' Drupal

  civicrm_domain.version := 5.23.alpha1

  Parsing schema description schema/Schema.xml
  Extracting database information
  Extracting table information
  PHP Fatal error:  Uncaught RuntimeException: Invalid configuration: the [cms.root] path must be an absolute URL in /home/me/.../Civi/Core/Paths.php:174
  Stack trace:
  #0 /home/me/.../Civi/Core/Paths.php(151): Civi\Core\Paths->toAbsoluteUrl('/', 'cms.root')
  #1 /home/me/.../Civi/Core/Paths.php(176): Civi\Core\Paths->getVariable('cms.root', 'url')
  #2 /home/me/.../Civi/Core/Paths.php(151): Civi\Core\Paths->toAbsoluteUrl('/', 'civicrm.root')
  civicrm#3 /home/me/.../Civi/Core/Paths.php(224): Civi\Core\Paths->getVariable('civicrm.root', 'path')
  civicrm#4 /home/me/.../Civi/Core/Paths.php(84): Civi\Core\Paths->getPath('l10n')
  civicrm#5 [internal function]: Civi\Core\Paths->Civi\Core\{closure}()
  civicrm#6 /home/jo in /home/me/.../Civi/Core/Paths.php on line 174
  ```

After
-----

You can run `./bin/setup.sh -g` with or without the `l10n` folder.

Comments
--------

There's an argument to be made that this shouldn't be necessary: when running
`GenCode`, it should only create PHP files, and none of the output should depend
on the CMS URL - because that's liable to change when you deploy the PHP code.
If someone did try to generate URL at such an early stage, it's arguably good to
generate an error.  In point of fact, `GenCode` is not actually using the CMS
URL.

The oddity stems from the contract of `CRM_Utils_System_*` (specifically,
`getCiviSourceStorage()` and `getDefaultFileStorage()`) which do double-duty
providing both path and URL.  To avoid duplicate work, `Civi\Core\Paths` uses
the same grain of information - it tracks the pairs of path+URL.

A more aggressive fix might be to split `getCiviSourceStorage()` and
`getDefaultFileStorage()` so that it's possible to get the path and URL
separately; then revise `Civi\Core\Paths` to take advantage of the finer-grained
contract.  However, splitting those things would be a more invasive patch,
and we're currently in RC.
@civibot
Copy link

civibot bot commented Feb 18, 2020

(Standard links)

@seamuslee001
Copy link
Contributor

(CiviCRM Review Template WORD-1.2)

@eileenmcnaughton eileenmcnaughton merged commit 428fe7b into civicrm:5.23 Feb 18, 2020
@totten totten deleted the 5.23-gencode-no-l10n branch February 18, 2020 07:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants