Moodle

Config.php require/_once usage not consistent

Details

  • Type: Improvement Improvement
  • Status: Closed Closed
  • Priority: Trivial Trivial
  • Resolution: Fixed
  • Affects Version/s: 1.9.4
  • Fix Version/s: 1.9.5
  • Component/s: Other
  • Labels:
    None
  • Affected Branches:
    MOODLE_19_STABLE
  • Fixed Branches:
    MOODLE_19_STABLE

Description

There are a few places that use require, rather than require_once, to include config.php.

While this does not cause any practical problems in normal usage it is inconsistent. Also it breaks a test script we use

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Sam Marshall added a comment -

Committed in HEAD, will commit in MOODLE_19 tomorrow as today is freeze day.

Show
Sam Marshall added a comment - Committed in HEAD, will commit in MOODLE_19 tomorrow as today is freeze day.
Hide
Permalink
Sam Marshall added a comment -

Fixed. I only changed require to require_once, no other change.

Show
Sam Marshall added a comment - Fixed. I only changed require to require_once, no other change.
Hide
Permalink
Petr Škoda (skodak) added a comment -

silly question, why do you use require_once() for script that should be required only once?
The only exception I can think of are scripts that allow key login or such similar hacks.

Show
Petr Škoda (skodak) added a comment - silly question, why do you use require_once() for script that should be required only once? The only exception I can think of are scripts that allow key login or such similar hacks.
Hide
Permalink
Petr Škoda (skodak) added a comment -

also what was the reason to do these cosmetic changes in stable branch?

Show
Petr Škoda (skodak) added a comment - also what was the reason to do these cosmetic changes in stable branch?
Hide
Permalink
Sam Marshall added a comment -

justification for checking in: well I thought I could get away with it because it was a bugfix imo, just on the ground of consistency, and there's really very little chance of problem from that change - safest to always use require_once even if you think it wouldn't happen again. there were only those like 10 places in the whole code. admittedly it is trivial though so this is sketchy.

real reason is obviously a local issue in that we want to avoid diffs, but I thought the justification above was just about reasonable, it's really not a very major change and [at least in some of those files which we do use] it's pretty well-tested because we are running it live right now...

We need it to not do that because we have a really useful admin-only script for analysing db performance on live server - basically you call this script, give it the url of a page you want to view, and it shows you that page but with $db->debug turned on. (Our db->debug shows time in milliseconds for each query too, can't remember if we already added that to core.) At times we see really different db performance on live vs acct systems so being able to check performance live is important. Anyhow, this script needs to do require_once config.php itself, and config.php breaks horribly if it is required twice.

Show
Sam Marshall added a comment - justification for checking in: well I thought I could get away with it because it was a bugfix imo, just on the ground of consistency, and there's really very little chance of problem from that change - safest to always use require_once even if you think it wouldn't happen again. there were only those like 10 places in the whole code. admittedly it is trivial though so this is sketchy. real reason is obviously a local issue in that we want to avoid diffs, but I thought the justification above was just about reasonable, it's really not a very major change and [at least in some of those files which we do use] it's pretty well-tested because we are running it live right now... We need it to not do that because we have a really useful admin-only script for analysing db performance on live server - basically you call this script, give it the url of a page you want to view, and it shows you that page but with $db->debug turned on. (Our db->debug shows time in milliseconds for each query too, can't remember if we already added that to core.) At times we see really different db performance on live vs acct systems so being able to check performance live is important. Anyhow, this script needs to do require_once config.php itself, and config.php breaks horribly if it is required twice.
Hide
Permalink
Petr Škoda (skodak) added a comment -

thanks for explanation, this is exactly the info I need to know when working on changes in HEAD

Show
Petr Škoda (skodak) added a comment - thanks for explanation, this is exactly the info I need to know when working on changes in HEAD

People

Vote (0)
Watch (1)

Dates

  • Created:
    Updated:
    Resolved:
Atlassian JIRA (v4.4.3#663-r165197) | Report a problem
Powered by a free Atlassian JIRA open source license for Moodle. Try JIRA - bug tracking software for your team.