-
-
Notifications
You must be signed in to change notification settings - Fork 825
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
(dev/drupal#10) Make civicrm-version.php self-sufficient #12113
(dev/drupal#10) Make civicrm-version.php self-sufficient #12113
Conversation
…i version and CMS name
The function has traditionally been in the global namespace, and it should stay there for backward compatibility. This patch looks big, but it's really just wrapping code inside new `namespace ... { ... }` blocks.
…namespace The new helper functions in this file shouldn't be used externally; this file is just an adapter for backward-compatibility. External callers should still use `CRM_Utils_System::version()`, `civicrmVersion()`, or `version.xml`.
In a typical environment, this is just an unnecessary file-load, and it makes it harder to (someday) remove/deprecate `civicrm-version.php`.
Ex 1: Running `drush` commands Ex 2: Running `cv` with `--level=cms-only`
Ex 1: Running `wp-cli` commands Ex 2: Running `cv` with `--level=cms-only`
Ex 1: Running `drush` commands Ex 2: Running `cv` with `--level=cms-only`
Thanks @totten . I have tested the patch and followed each step you have mentioned. Checked with rebuilt tarball and its working fine. |
@eileenmcnaughton @colemanw - A procedural question on merging: Monish and I have both put in SLOC/commits here, and we've both reviewed the others' commit. On the merits, I think it's seen a good chunk of review, but hitting "Merge" would technically be a self-merge, so I want to make sure the record is persuasive. You think we're OK to merge? Or should we put it through some other step? |
@totten right - this is a constant issue - when we collaborate we are implicitly reviewing each other's work but then we don't feel able to merge the result. I have experienced this problem a lot & TBH what I do is convince the other party to resubmit the whole lot with my commits squashed in to pass the letter of the law without it being meaningfully different. I don't have an answer to the bigger issue - but I will merge this based on the knowledge that you @monishdeb are satisified with it that is enough for me. |
Overview
The file
civicrm-version.php
has traditionally been a generated file. However, this is an impediment to composer-based deployment. The PR aims to allow one to use a static, non-generated copy ofcivicrm-version.php
.This PR is a continuation of @monishdeb's #12016 -- while testing that PR, I ran into various issues which were cumulatively easier to patch than to discuss. Each is presented as a separate commit (with description) in this PR.
Before
civicrm-version.php
generated bydistmaker
.civicrm-version.php
generated byGenCode
.After
civicrm-version.php
includes a deprecation notice.civicrm-version.php
generated bydistmaker
(same as before).civicrm-version.php
from git. This file will:xml/version.xml
Technical Details
The static file relies on CMS autodetection -- it should work in most cases, but fundamentally autodetection depends on the environment, and there are some cases where CMS autodetection doesn't work. This will arguably be a compatibility break for someone, but there's a number of mitigating factors.
universe
, almost nobody relies oncivicrm-version.php
to report the CMS -- the only known use-cases are (a) displaying an informational field in the installation UI and (b) thedrush civicrm-install
command (for Drupal variants).civicrm-version.php
in multiple CMS's (D7/WP/BD) and in multiple uses (listed below).distmaker
.So I spent a lot of time on testing
civicrm-version.php
, namely doing each of these in D7/WP/BD:civicrm-version.php
with adhoc PHP in an unbooted environmentphp -r 'require "civicrm-version.php"; $a = civicrmVersion(); print_r($a);'
(borrowing a bit fromcivi-test-run
)civicrm-version.php
through an intermediate file (/tmp/ver.php
which does the same as above) in an unbooted environmentphp /tmp/ver.php
civicrm-version.php
with adhoc PHP in a booted environmentdrupal ev 'require "'$PWD'/civicrm-version.php"; $a = civicrmVersion(); print_r($a);'
cv ev --level=cms-only 'require "'$PWD'/civicrm-version.php"; $a = civicrmVersion(); print_r($a);'
civicrm-version.php
through an intermediate file (/tmp/ver.php
) in a booted environmentdrush scr /tmp/ver.php
wp eval-file /tmp/ver.php
cv scr --level=cms-only /tmp/ver.php
drush civicrm-install
drush civicrm-install
would work.)The static
civicrm-version.php
reported version# correctly in all scenarios that I tested. However, the CMS reporting is a little flaky with unbooted environments (a+b) because they depend on a filesystem scan -- but the start-point for the filesystem scan depends on happenstance (how exactly you call it) and the validity of the scan depends on your system configuration.This is arguably a compatibility break, but honestly -- I'd rather kill the CMS-reporting than spend any more time getting the CMS-reporting to work in unbooted environments. It's just not used much, and it's brittle by design. That's why I've marked
civicrmVersion()
as deprecated. But for the moment - normal tarball users should be getting exactly the same as before, and git/composer users should have something that works in the real/known use-cases (e,f).