CiviCRM Community Forums (archive)

During my work on CRM-6492 (multi-domain support). I saw that the code to generate certain URLs or paths is scattered across a lot of modules and even duplicated some times. I propose to factor out this code into it's own module (CRM_Utils_Url ?). This would be a nice excercise for me to get to know the code better.

As overall structure to avoid the rather ugly eval-hacks that are often used in CRM_Utils_System I propose something like this (simplified):


class CRM_Utils_Url {
  /** constructor to be called from UF-independent code */
  static function instance() {
    return new CRM_Utils_Url_$userFramework();
  }
  /* generic code here */
}

class CRM_Utils_Url_Drupal extends CRM_Utils_Url {
  /* Drupal specific code here */
}

The url module could then be accessed by using a static property of CRM_Utils_System:
CRM_Utils_System::$url->someMethod()

No need to use $config->userFrameworkClass here.

The migration steps would be:

• implement the new classes - and unittests for them
• make the old methods simple proxies for the new methods - mark the old methods as deprecated
• change the rest of the code to use the new classes directly
• remove the old methods

What do you think?

There are two more function that do some URL generating: makeURL() and getLinkUrl() (both in CRM_Utils_System). getLinkUrl() is only used by makeURL(). makeURL() is only used twice throughout CiviCRM. It's probably easy to replace those calls with calls to Url. (I attached a patch to remove them. - Still needs testing though.)

The constant CIVICRM_UF_BASEURL is used once outside of CRM_Core_Config.
$config->userFrameworkURL is used in a lot of files outside of System - namely:
./tests/phpunit/CiviTest/CiviTestCase.php
./bin/migrate/move.php
./CRM/Contribute/Page/PCPInfo.php
./CRM/Contribute/BAO/PCP.php
./CRM/Contribute/Form/ContributionPage/Widget.php
./CRM/Contribute/Form/ManagePremiums.php
./CRM/Upgrade/3.1.alpha2.msg_template/message_templates/pcp_status_change_html.tpl
./CRM/Upgrade/3.1.alpha1.msg_template/message_templates/pcp_status_change_html.tpl
./CRM/Upgrade/3.1.alpha1.msg_template/message_templates/pcp_status_change_text.tpl
./CRM/Core/Controller.php
./CRM/Core/QuickForm/Action.php
./CRM/Core/BAO/CMSUser.php
./CRM/Core/BAO/Setting.php
./CRM/Core/Config.php
./CRM/Core/Session.php
./CRM/Core/Config/Variables.php
./CRM/Core/Config/Defaults.php
./CRM/UF/Page/Group.php
./CRM/Event/BAO/Event.php
./CRM/Event/Form/Registration/ParticipantConfirm.php
./CRM/Utils/VersionCheck.php
./CRM/Contact/Page/View.php
./CRM/Contact/BAO/Contact.php
./CRM/Contact/BAO/Contact/Permission.php
./CRM/Mailing/Page/Common.php
./CRM/Mailing/Form/ForwardMailing.php
./templates/CRM/Admin/Page/ConfigTaskList.tpl
./templates/CRM/Core/Calendar/Rss.tpl
./templates/CRM/common/civicrm_variables.tpl
./templates/CRM/common/CMSUser.tpl
./templates/CRM/common/fatal.tpl
./standalone/index.php
./joomla/site/civicrm.php
At least some of those usages - like ./CRM/Contact/BAO/Contact.php - seem to be for URL generation. Though I must admit the situation seemed to be worse at the first glance.

The ::url function of CRM_Core_System_{Drupal,Standalone,UnitTests} is identical. Also it does some work that rather could be part of $config or another function that is only run once for every request.

Why do I think eval is bad?
 * Someone who can manipulate the value of any variable that is used in the eval statements can run arbitrary code.
 * It's likely to harm performance because the parser has to be invoked again for each eval.
 * In this case it is IMHO used for something that seems to be a typical case for the factory pattern. After all PHP is a OO language.
 * There is a way to do the very same thing without evals (take a look at http://www.php.net/manual/en/language.oop5.patterns.php )

Also at least the System and Config classes seem programmed in a procedural style. One of the symptoms is that ::url has so many parameters. For me it looks like it's harder to write unit tests for those functions than it should be.

Please don't misunderstand my criticism. I am just looking for ways to make myself useful

roman

ad 1. thanks
ad 2. why don't we use inheritance to avoid that kind of code duplication?
ad 4. there is no hurry. I'm probably going to work more with civicrm in the future so I plan to be around when those changes hit stable.

ad 3. sure I will contact you on IRC to discuss the details. see you there.