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

      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

          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 Skoda added a comment - - edited

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

            Show
            Petr Skoda 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 Skoda added a comment -

            +1

            Show
            Petr Skoda 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: