Uploaded image for project: '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
    • Status: Closed
    • Priority: 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

      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?)

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Hide
              salvetore 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
              salvetore 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
              dougiamas 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
              dougiamas 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
              stronk7 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
              stronk7 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
              dougiamas Martin Dougiamas added a comment -

              Marking blocker just to get it dealt with asap.

              Show
              dougiamas Martin Dougiamas added a comment - Marking blocker just to get it dealt with asap.
              Hide
              nebgor 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
              nebgor 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
              nebgor 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
              nebgor 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
              salvetore Michael de Raadt added a comment -

              Carrying over to new sprint.

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

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

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

              Doesn't look rebased to me Apu?

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

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

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

              Show
              skodak Petr Skoda added a comment - - edited Why not simply add into lib/setup.php something like <code>$CFG->moodlebranch = '2.2'<code>
              Hide
              stronk7 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
              stronk7 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
              nebgor 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
              nebgor 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
              stronk7 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
              stronk7 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
              stronk7 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
              stronk7 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
              andyjdavis 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
              andyjdavis 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
              mudrd8mz David Mudrák 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
              mudrd8mz David Mudrák 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
              nebgor Aparup Banerjee added a comment -

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

              Show
              nebgor Aparup Banerjee added a comment - ok i'll leave this in integrators/ peer-reviewers, but heres the proposal , for better or for worse
              Hide
              dougiamas 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
              dougiamas 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
              nebgor 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
              nebgor 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
              mudrd8mz David Mudrák added a comment -

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

              Show
              mudrd8mz David Mudrák added a comment - This seems to be pretty elegant solution. Thanks Martin and Apu.
              Hide
              skodak Petr Skoda added a comment -

              +1

              Show
              skodak Petr Skoda added a comment - +1
              Hide
              poltawski 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
              poltawski 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
              nebgor 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
              nebgor 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
              poltawski 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
              poltawski 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
              poltawski 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
              poltawski 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
              nebgor 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
              nebgor 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
              poltawski Dan Poltawski added a comment -

              Done

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

              Successfully tested on master

              Show
              fred Frédéric Massart added a comment - Successfully tested on master
              Hide
              stronk7 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
              stronk7 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:
                    Fix Release Date:
                    25/Jun/12