dev/civicrm-setup#11 Remove templates/CRM/common/version.tpl

[monishdeb]

Overview

For civicrm/civicrm-setup#11, the aim is to install the database schema without needing to run GenCode -- and templates/CRM/common/version.tpl is generated to by GenCode as an intermediate step toward producing civicrm-version.php (revision). This PR removes templates/CRM/common/version.tpl.

If you grep universe, no other projects reference CRM/common/version.tpl. It's only used to hold the Civi version later assigned to $svnversion/revision. In turn, no other projects reference the revision field.

Before

• CRM_Core_CodeGen_Version writes the Civi version to templates/CRM/common/version.tpl
• Then CRM_Core_CodeGen_Version evaluates xml/civicrm_version.tpl and templates/CRM/common/version.tpl -- and writes the content to civicrm-version.php.

After

• The file templates/CRM/common/version.tpl has been thoroughly removed:
  • CRM_Core_CodeGen_Version does not write the Civi version to templates/CRM/common/version.tpl
  • xml/civicrm_version.tpl does not consume templates/CRM/common/version.tpl or output an $svnVersion/revision. (The revision is not referenced elsewhere in universe.)
  • CRM_Utils_Check_Component_Source::getRemovedFiles() ensures that the file is no longer present in the codebase.
  • Removed the path reference from .gitignore
• The file civicrm-version.php is still available for consumption by CRM_Utils_System::version() and others.

[monishdeb]

ping @totten @seamuslee001

[monishdeb]

On the other hand I am not sure, why we need to have generated civicrm-version.php file as we can still fetch the version from xml/version.xml then why we need to keep this php to use civicrmVersion(). So here's the definition of CRM_Utils_System::version()

/**
   * Return the running civicrm version.
   *
   * @return string
   *   civicrm version
   */
  public static function version() {
    static $version;

    if (!$version) {
      $verFile = implode(DIRECTORY_SEPARATOR,
        array(dirname(__FILE__), '..', '..', 'xml', 'version.xml')
      );
      if (file_exists($verFile)) {
        $str = file_get_contents($verFile);
        $xmlObj = simplexml_load_string($str);
        $version = (string) $xmlObj->version_no;
      }

      // pattern check
      if (!CRM_Utils_System::isVersionFormatValid($version)) {
        CRM_Core_Error::fatal('Unknown codebase version.');
      }
    }

    return $version;
  }

If we approve this, then we no longer need CRM_Core_CodeGen_Version file and also resolve civicrm/civicrm-setup#12 Or am I missing something here ?

[totten]

Based on grepping the known universe, nothing else (outside the items in this PR) references templates/CRM/common/version.tpl. So it seems pretty safe to remove. It feels like a left-over artifact from how footers were rendered long-ago.

Let me r-run the patch before merging.

@monishdeb, I'm glad you mentioned the point about reading version.xml directly. Let me respond in https://lab.civicrm.org/dev/drupal/issues/10 (since that's focused more pointedly on civicrm-version.php).

[totten]

(CiviCRM Review Template WORD-1.0)

• (r-explain) Needs work: We don't have a JIRA issue, but we do have some ref's. But I think we should at least follow the standard pull-request template (with before+after sections).
• (r-test) Pass
• (r-code) Pass
• (r-doc) Pass
• (r-maint) Pass
• (r-run) Pass: Did an r-run locally and confirmed that (a) the file templates/CRM/common/version.tpl is not created and (b) the file civicrm-version.php is created and (c) the page-footer looks right and (d) the System.get API looks right.
• (r-user) Pass: Does not affect a user-visible feature.
• (r-tech) Pass: Based on grepping the known universe, nothing else (outside the items in this PR) references templates/CRM/common/version.tpl.

[totten]

Oh, technically, deleting a file is tricky (which is why CRM_Utils_Check_Component_Source::getRemovedFiles()exists), although there's no real functional problem or security problem if this particular file exists.

I'd say the main thing needed to merge is to clean-up the description a bit. But if you also think it's good to update CRM_Utils_Check_Component_Source, then go for it.

[monishdeb]

@totten I have updated the PR description and CRM_Utils_Check_Component_Source I will submit a new PR for https://lab.civicrm.org/dev/drupal/issues/10

I didn't create JIRA issue for this PR as I saw somewhere (maybe on mergers channel) that having JIRA for each PR isn't necessary until and unless it got references to relate with and has milestone set. In this case, I was not able to choose milestone as it only got 4.7.31. For future PRs on civicrm-setup issues, I will make sure to have corresponding JIRA ticket

[totten]

Thank you!

Agree we don't need a JIRA issue -- we're just at a funny transitional moment because we haven't fully replaced the r-jira criterion (e.g. r-explain is still a draft).

I made a few more copy-edits on the description.

The new test failures are common false-negatives.