Moodle
  1. Moodle
  2. MDL-28134

relying on $release and regex to get the version to point to within docs too dependant on formatting of release.

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 1.9.13, 2.0.4, 2.1
    • Fix Version/s: 2.3
    • Component/s: General
    • Labels:
    • Testing Instructions:
      Hide

      1)start a fresh installation of moodle
      2)on the installation pages, check that all links to docs.moodle.org (everypage has one at the bottom) are correct for the version of moodle.
      3) after installation, check that pages have the correct link to its docs page - correct version too.

      test on web as well as cli based installations/upgrades.

      Show
      1)start a fresh installation of moodle 2)on the installation pages, check that all links to docs.moodle.org (everypage has one at the bottom) are correct for the version of moodle. 3) after installation, check that pages have the correct link to its docs page - correct version too. test on web as well as cli based installations/upgrades.
    • Affected Branches:
      MOODLE_19_STABLE, MOODLE_20_STABLE, MOODLE_21_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-28134_branch
    • Rank:
      17752

      Description

      mentioned in MDL-28044 : http://tracker.moodle.org/browse/MDL-28044?focusedCommentId=115425&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-115425

      Eloy said:
      "3) And the most I think on it... the less I like (this applies to all branches, not only 19_STABLE) the include of version.php at all nor the parsing of $release to calculate $branch. After all, each branch has different code and we could have some constant/define with the 19/20... That would make everything really easy IMO.
      .. about the 19_STABLE problem and the version.php consideration. Just imagine we invent version 10.1 or whatever, that will break the regexp, while having it in some constant will do things really easier and free of problems loading version.php multiple times or so."

      it certainly makes more sense to define a specific branch variable to me. (sole purpose is for linking to docs initially?)

        Issue Links

          Activity

          Hide
          Michael de Raadt added a comment -

          Better get on to this one. At current pace we only have 50 years to resolve this issue.

          (Isn't that what they said about two digit years?)

          Show
          Michael de Raadt added a comment - Better get on to this one. At current pace we only have 50 years to resolve this issue. (Isn't that what they said about two digit years?)
          Hide
          Martin Dougiamas added a comment -

          I'm not too worried about this. Parsing $release -> $branch seems like it will be easy and accurate for a long time yet. Otherwise we have another thing to manually check and add on every release.

          Show
          Martin Dougiamas added a comment - I'm not too worried about this. Parsing $release -> $branch seems like it will be easy and accurate for a long time yet. Otherwise we have another thing to manually check and add on every release.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Well, I agree it can continue working for ages, but also think it's a "prone to error" thing to be including that file, that is also included on install/upgrade and could lead, at some point, to double inclusion error in some installer.

          Anyway, IMO, both this and MDL-28135 should go one step forward and properly define what we want on each case, language and branch situation.

          For example, it's assumed that the "manual" CFG variables always are adding the branch/en thingy. Always, and that's a nono. What if I don't want them in my own Docs?

          Hence, I created one simpletest, both for docs links and error links:

          https://github.com/stronk7/moodle/compare/master...doclinks_crazy_tests

          And the fist step (after release, for sure), should be to define what we want there. And then, code it. Right now it represents my vision of the links, just that. But it (the test) must be agreed and completed. Then, coded.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Well, I agree it can continue working for ages, but also think it's a "prone to error" thing to be including that file, that is also included on install/upgrade and could lead, at some point, to double inclusion error in some installer. Anyway, IMO, both this and MDL-28135 should go one step forward and properly define what we want on each case, language and branch situation. For example, it's assumed that the "manual" CFG variables always are adding the branch/en thingy. Always, and that's a nono. What if I don't want them in my own Docs? Hence, I created one simpletest, both for docs links and error links: https://github.com/stronk7/moodle/compare/master...doclinks_crazy_tests And the fist step (after release, for sure), should be to define what we want there. And then, code it. Right now it represents my vision of the links, just that. But it (the test) must be agreed and completed. Then, coded. Ciao
          Hide
          Martin Dougiamas added a comment -

          Marking blocker just to get it dealt with asap.

          Show
          Martin Dougiamas added a comment - Marking blocker just to get it dealt with asap.
          Hide
          Aparup Banerjee added a comment -

          Hello, just looking at this now. I'm getting 404 on https://github.com/stronk7/moodle/compare/master...doclinks_crazy_tests

          Show
          Aparup Banerjee added a comment - Hello, just looking at this now. I'm getting 404 on https://github.com/stronk7/moodle/compare/master...doclinks_crazy_tests
          Hide
          Aparup Banerjee added a comment -

          hopefully Eloy can find his doclinks_crazy_tests meanwhile i've put up a wip branch - adding $moodle=2 and $revision=3 (master) to help build the $branch

          Show
          Aparup Banerjee added a comment - hopefully Eloy can find his doclinks_crazy_tests meanwhile i've put up a wip branch - adding $moodle=2 and $revision=3 (master) to help build the $branch
          Hide
          Michael de Raadt added a comment -

          Carrying over to new sprint.

          Show
          Michael de Raadt added a comment - Carrying over to new sprint.
          Hide
          Aparup Banerjee added a comment -

          up for peer-review. (should have sent it awhile ago now, so rebased too)

          Show
          Aparup Banerjee added a comment - up for peer-review. (should have sent it awhile ago now, so rebased too)
          Hide
          Dan Poltawski added a comment -

          Doesn't look rebased to me Apu?

          BTW we have a related issue to this - MDL-31933

          Show
          Dan Poltawski added a comment - Doesn't look rebased to me Apu? BTW we have a related issue to this - MDL-31933
          Hide
          Petr Škoda added a comment - - edited

          Why not simply add into lib/setup.php something like <code>$CFG->moodlebranch = '2.2'<code>

          Show
          Petr Škoda added a comment - - edited Why not simply add into lib/setup.php something like <code>$CFG->moodlebranch = '2.2'<code>
          Hide
          Eloy Lafuente (stronk7) added a comment -

          More yet, why not, simply (@ setuplib.php warmup):

          $CFG->moodlebranch = 'master|20|21|22|23';
          

          and done? It's exactly what it is (the branch!), and don't need any processing. And we only need to change it each time we branch (Major release notes).

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - More yet, why not, simply (@ setuplib.php warmup): $CFG->moodlebranch = 'master|20|21|22|23'; and done? It's exactly what it is (the branch!), and don't need any processing. And we only need to change it each time we branch (Major release notes). Ciao
          Hide
          Aparup Banerjee added a comment - - edited

          Petr, i tried no dots like in '2.2' since that would require regex for any future processing.

          Eloy, that seems fine except that it seems like counting , 19,20,21,22,23...(29?),30 - when 29 doesn't happen it may seem confusing. So i thought split the two numbers for clarity. and then combine them. further clarity/future proof. Edit: are did i get that wrong and we ARE counting branches?

          Dan, grr forgot to update my copy of master on github so it doesn't seem rebased but the branch itself is rebased against moodle.git master - i'll update my github master copy very soon.

          Show
          Aparup Banerjee added a comment - - edited Petr, i tried no dots like in '2.2' since that would require regex for any future processing. Eloy, that seems fine except that it seems like counting , 19,20,21,22,23...(29?),30 - when 29 doesn't happen it may seem confusing. So i thought split the two numbers for clarity. and then combine them. further clarity/future proof. Edit: are did i get that wrong and we ARE counting branches? Dan, grr forgot to update my copy of master on github so it doesn't seem rebased but the branch itself is rebased against moodle.git master - i'll update my github master copy very soon.
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          (pipe) = or (list of possible values). We only will put ONE of those.

          Anyway... adding David here... moment...

          Show
          Eloy Lafuente (stronk7) added a comment - - edited (pipe) = or (list of possible values). We only will put ONE of those. Anyway... adding David here... moment...
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Just for reference, he pointed me to this:

          https://github.com/mudrd8mz/moodle/commit/2548002be02eb7c23097df4f5816aa6d6c74e532

          that is used both for langs and will be also used for the update-checker. It continues relying on regex and the main difference is that it returns dotted major version instead of (undotted) branch number.

          For your consideration, perhaps our $CFG->moodlebranch could rely on that (dots out) or David's function is enough, or whatever.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Just for reference, he pointed me to this: https://github.com/mudrd8mz/moodle/commit/2548002be02eb7c23097df4f5816aa6d6c74e532 that is used both for langs and will be also used for the update-checker. It continues relying on regex and the main difference is that it returns dotted major version instead of (undotted) branch number. For your consideration, perhaps our $CFG->moodlebranch could rely on that (dots out) or David's function is enough, or whatever. Ciao
          Hide
          Andrew Davis added a comment -

          The changes look ok I suppose.

          Are there better names possible for the variables that are being introduced? If you read this aloud it makes no sense.
          $release = $moodle.'.'.$revision;

          Perhaps $majorversion, $minorversion or something.

          Given that we are already changing the human readable version string (from "2.3dev (Build: 20120315)" to "2.3development (Build: 20120315)" is it worth putting a space before "development"?

          Show
          Andrew Davis added a comment - The changes look ok I suppose. Are there better names possible for the variables that are being introduced? If you read this aloud it makes no sense. $release = $moodle.'.'.$revision; Perhaps $majorversion, $minorversion or something. Given that we are already changing the human readable version string (from "2.3dev (Build: 20120315)" to "2.3development (Build: 20120315)" is it worth putting a space before "development"?
          Hide
          David Mudrak added a comment -

          Hmm. I am not much aware of the plans to change "2.3dev (Build: 20120315)" to "2.3development (Build: 20120315)" but please note that the new update notifications stuff is going to land to master soon and it relies massively on the current syntax of $version and $release in version.php. See the unit tests for it for examples.

          Show
          David Mudrak added a comment - Hmm. I am not much aware of the plans to change "2.3dev (Build: 20120315)" to "2.3development (Build: 20120315)" but please note that the new update notifications stuff is going to land to master soon and it relies massively on the current syntax of $version and $release in version.php. See the unit tests for it for examples.
          Hide
          Aparup Banerjee added a comment -

          ok i'll leave this in integrators/ peer-reviewers, but heres the proposal , for better or for worse

          Show
          Aparup Banerjee added a comment - ok i'll leave this in integrators/ peer-reviewers, but heres the proposal , for better or for worse
          Hide
          Martin Dougiamas added a comment -

          My +1 to not change $release or $version AT ALL, but just add a $branch variable in version.php to solve the actual original problem (if in fact there even was one).

          So version.php has:

          $version = 2011120501.06;
          $release = '2.2.1+ (Build: 20120213)';
          $branch = '22';
          $maturity = MATURITY_STABLE;

          Show
          Martin Dougiamas added a comment - My +1 to not change $release or $version AT ALL, but just add a $branch variable in version.php to solve the actual original problem (if in fact there even was one). So version.php has: $version = 2011120501.06; $release = '2.2.1+ (Build: 20120213)'; $branch = '22'; $maturity = MATURITY_STABLE;
          Hide
          Aparup Banerjee added a comment -

          ok i've just added the $branch='xx' into version.php. $CFG->branch is also made now.

          21, 22 and master branches are up for integration review now.

          Show
          Aparup Banerjee added a comment - ok i've just added the $branch='xx' into version.php. $CFG->branch is also made now. 21, 22 and master branches are up for integration review now.
          Hide
          David Mudrak added a comment -

          This seems to be pretty elegant solution. Thanks Martin and Apu.

          Show
          David Mudrak added a comment - This seems to be pretty elegant solution. Thanks Martin and Apu.
          Hide
          Petr Škoda added a comment -

          +1

          Show
          Petr Škoda added a comment - +1
          Hide
          Dan Poltawski added a comment -

          Hi - should we really be backporting this?

          To be honest - despite reading this whole issue i'm still unclear of what the problem is!

          Show
          Dan Poltawski added a comment - Hi - should we really be backporting this? To be honest - despite reading this whole issue i'm still unclear of what the problem is!
          Hide
          Aparup Banerjee added a comment -

          Hi Dan,
          this really isn't a case of a problem but what could be a problem (in the future). That the branch formats change or go above 2 digits and there is regex around that rely's on a fixed branch length+format by parsing $release. This patch basically makes life a little more straight forward.

          i don't think the back port is really important. Its there simply because it can be considered and i couldn't see a problem with existing installations by back porting it.

          Show
          Aparup Banerjee added a comment - Hi Dan, this really isn't a case of a problem but what could be a problem (in the future). That the branch formats change or go above 2 digits and there is regex around that rely's on a fixed branch length+format by parsing $release. This patch basically makes life a little more straight forward. i don't think the back port is really important. Its there simply because it can be considered and i couldn't see a problem with existing installations by back porting it.
          Hide
          Dan Poltawski added a comment -

          I've integrated this now - only to master as it doesn't seem there is a requirement to backport.

          Show
          Dan Poltawski added a comment - I've integrated this now - only to master as it doesn't seem there is a requirement to backport.
          Hide
          Dan Poltawski added a comment -

          Just noticed on install:

          Notice: Undefined property: stdClass::$branch in /Users/danp/git/integration/admin/index.php on line 264 Call Stack: 0.0001 638712 1.

          {main}

          () /Users/danp/git/integration/admin/index.php:0

          Show
          Dan Poltawski added a comment - Just noticed on install: Notice: Undefined property: stdClass::$branch in /Users/danp/git/integration/admin/index.php on line 264 Call Stack: 0.0001 638712 1. {main} () /Users/danp/git/integration/admin/index.php:0
          Hide
          Aparup Banerjee added a comment - - edited

          Dan, could you please pick the last commit from https://github.com/nebgor/moodle/compare/mistress...MDL-28134_branch
          which is https://github.com/nebgor/moodle/commit/ad394588db002e3ef8a0727f960ac93c6e39bf7d

          tested on web & cli installations: notice gone.

          Show
          Aparup Banerjee added a comment - - edited Dan, could you please pick the last commit from https://github.com/nebgor/moodle/compare/mistress...MDL-28134_branch which is https://github.com/nebgor/moodle/commit/ad394588db002e3ef8a0727f960ac93c6e39bf7d tested on web & cli installations: notice gone.
          Hide
          Dan Poltawski added a comment -

          Done

          Show
          Dan Poltawski added a comment - Done
          Hide
          Frédéric Massart added a comment -

          Successfully tested on master

          Show
          Frédéric Massart added a comment - Successfully tested on master
          Hide
          Eloy Lafuente (stronk7) added a comment -

          This is now part of Moodle and a few millions people around the globe will be using it soon. Isn't that awesome?

          Many, many thanks and don't forget http://youtu.be/4N7dPaP5Z8U

          Closing, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - This is now part of Moodle and a few millions people around the globe will be using it soon. Isn't that awesome? Many, many thanks and don't forget http://youtu.be/4N7dPaP5Z8U Closing, ciao

            People

            • Votes:
              0 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: