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

CRM-21787 : Simplify CRM_Utils_System::version() to fetch version directly from xml/version.xml #11700

Merged
merged 3 commits into from
Feb 21, 2018

Conversation

monishdeb
Copy link
Member

@monishdeb monishdeb commented Feb 21, 2018

Overview

This patch simplifies CRM_Utils_System::version() by removing a level of indirection -- instead of reading xml/version.xml indirectly (by way of civicrm-version.php and GenCode), it reads xml/version.xml directly.

This change is valid now that xml/version.xml is included with tarballs (04e5df1).

Before

CRM_Utils_System::version() reads civicrm-version.php OR xml/version.xml.

After

CRM_Utils_System::version() reads xml/version.xml.

Technical Details

To avoid CRM_Core_CodeGen_Version dependency to generate civicrm-version.php is to make this file independent, to retrieve the cms and version on its own. For version, it's possible to directly use xml/version.xml but for cms we need to rely on php constant CIVICRM_UF. Then we can easily remove CRM_Core_CodeGen_Version. (Outside scope of this PR.)

Comments

This PR contains commits of #11695


@monishdeb
Copy link
Member Author

ping @totten

@totten
Copy link
Member

totten commented Feb 21, 2018

@monishdeb I updated the description to simplify the prose and remove the comments about cms/CIVICRM_UF. Why? Because (a) this patch (e48985c) focuses on CRM_Utils_System::version() which never cared about the cms or CIVICRM_UF, and (b) if we were talking about civicrm-version.php, CIVICRM_UF wouldn't be a substitute for cms. (But we can talk about that more in an issue or PR that focuses on civicrm-version.php.)

@totten totten merged commit 5ad79e0 into civicrm:master Feb 21, 2018
@monishdeb monishdeb deleted the CRM-21787 branch February 21, 2018 17:38
@monishdeb
Copy link
Member Author

Yeah agree with the edit, which was not appropriate to drag the topic of making civicrm-version.php sel-sufficient, in this PR.

@mlutfy mlutfy added this to the 4.7.32 milestone Mar 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants