Moodle
  1. Moodle
  2. MDL-38382

Update seems to truncate minor revisions

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Trivial Trivial
    • Resolution: Fixed
    • Affects Version/s: 2.3.4, 2.4.1, 2.5
    • Fix Version/s: 2.5
    • Component/s: Administration
    • Labels:
    • Testing Instructions:
      Hide
      1. Change micro increment of site version number (in version.php) to .10 (eg: 2013022800.00 to 2013022800.10)
      2. log in as admin and you should see upgrade page
      3. Make sure leading zero's are visible in header string and notification string. Something like

        Upgrading Moodle database from version 2.5dev (Build: 20130228) (2013022800.00) to 2.5dev (Build: 20130228) (2013022800.10)
        

      Show
      Change micro increment of site version number (in version.php) to .10 (eg: 2013022800.00 to 2013022800.10) log in as admin and you should see upgrade page Make sure leading zero's are visible in header string and notification string. Something like Upgrading Moodle database from version 2.5dev (Build: 20130228) (2013022800.00) to 2.5dev (Build: 20130228) (2013022800.10)
    • Affected Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE, MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull Master Branch:
      wip-mdl-38382

      Description

      Whilst testing MDL-38378 I cherry picked version version 2.3.4+ (Build: 20130228) (2012062504.10) to test the changes I made within '/lib/db/update.php' as the script has the condition:

      if ($oldversion < 2012062504.11 && $oldversion >= 2012062504.08) {
      

      So I thought a test value of 2012062504.10 would satisfy this criteria. However, the value after the decimal point appears to be truncated to 2012062504.1. And Moodle reports:

      Upgrading Moodle database from version 2.3.4+ (Build: 20130228) (2012062504.1) to 2.3.4+ (Build: 20130307) (2012062504.12)

      Note the disparity between the build date and reported Moodle version when compared with the original version.php file. Thus causing confusion on the part of the administrator / developer when using the value.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Petr Skoda added a comment -

            The version numbers are floats, you should not use "=" comparison for floats.

            Show
            Petr Skoda added a comment - The version numbers are floats, you should not use "=" comparison for floats.
            Hide
            Gareth J Barnard added a comment -

            Dear Petr Skoda,

            That does not account for the condition code in MDL-38378 as it is:

            $oldversion < 2012062504.11
            

            and the message upon upgrade of:

            Upgrading Moodle database from version 2.3.4+ (Build: 20130228) (2012062504.1) to 2.3.4+ (Build: 20130307) (2012062504.12)

            As the value of $oldversion is almost certainly being truncated.

            Cheers,

            Gareth

            Show
            Gareth J Barnard added a comment - Dear Petr Skoda , That does not account for the condition code in MDL-38378 as it is: $oldversion < 2012062504.11 and the message upon upgrade of: Upgrading Moodle database from version 2.3.4+ (Build: 20130228) (2012062504.1) to 2.3.4+ (Build: 20130307) (2012062504.12) As the value of $oldversion is almost certainly being truncated. Cheers, Gareth
            Hide
            Gareth J Barnard added a comment -

            I'm going to repeat the test to check.

            Show
            Gareth J Barnard added a comment - I'm going to repeat the test to check.
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Hi Gareth,

            first of all many thanks for your promptly action with MDL-38378. I read that query so many times that cannot believe how I skipped the prefix use.

            About this... I don't get the point... why do you say that $oldversion is being truncated? They are floats so, 2012062504.1 and 2012062504.10 are "equivalent", the former is not any truncation of the later.

            Hence, any of them is smaller than 2012062504.11, and the condition works. Or am I missing anything?

            Perhaps it would be better to show those versions with the trailing zeros, agree, but that is only cosmetic, it has no incidence in the evaluation afaik.

            Ciao

            Show
            Eloy Lafuente (stronk7) added a comment - Hi Gareth, first of all many thanks for your promptly action with MDL-38378 . I read that query so many times that cannot believe how I skipped the prefix use. About this... I don't get the point... why do you say that $oldversion is being truncated? They are floats so, 2012062504.1 and 2012062504.10 are "equivalent", the former is not any truncation of the later. Hence, any of them is smaller than 2012062504.11, and the condition works. Or am I missing anything? Perhaps it would be better to show those versions with the trailing zeros, agree, but that is only cosmetic, it has no incidence in the evaluation afaik. Ciao
            Hide
            Gareth J Barnard added a comment - - edited

            Dear Eloy Lafuente (stronk7),

            No worries

            I'm just double checking my test procedure in detecting this as real thing or just a cosmetic.

            Cheers,

            Gareth

            Show
            Gareth J Barnard added a comment - - edited Dear Eloy Lafuente (stronk7) , No worries I'm just double checking my test procedure in detecting this as real thing or just a cosmetic. Cheers, Gareth
            Hide
            Gareth J Barnard added a comment -

            Dear Eloy Lafuente (stronk7),

            In double checking (as you do), the script is running and hence 2012062504.10 is contained in $oldversion. It's the 'echo' of the value caused confusion. Therefore downgraded this to a 'trivial' as only cosmetic but could cause mild heart palpitations for admins.

            Going to reword the issue.

            Cheers,

            Gareth

            Show
            Gareth J Barnard added a comment - Dear Eloy Lafuente (stronk7) , In double checking (as you do), the script is running and hence 2012062504.10 is contained in $oldversion. It's the 'echo' of the value caused confusion. Therefore downgraded this to a 'trivial' as only cosmetic but could cause mild heart palpitations for admins. Going to reword the issue. Cheers, Gareth
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Aha, okey, agree, thanks!

            Show
            Eloy Lafuente (stronk7) added a comment - Aha, okey, agree, thanks!
            Hide
            Gareth J Barnard added a comment -

            Looks like fix for this is to use the 'p()' function and not what is being used at the moment.

            Show
            Gareth J Barnard added a comment - Looks like fix for this is to use the 'p()' function and not what is being used at the moment.
            Hide
            Rajesh Taneja added a comment -

            Thanks for reporting this Gareth,

            I have put this on backlog.

            In the meantime feel free to help us work on this issue. If you are able to provide a patch or links to your Git repository branch, please add a patch label so we will spot it.

            Show
            Rajesh Taneja added a comment - Thanks for reporting this Gareth, I have put this on backlog. In the meantime feel free to help us work on this issue. If you are able to provide a patch or links to your Git repository branch, please add a patch label so we will spot it.
            Hide
            Petr Skoda added a comment -

            +1 for he patch (master only)

            Show
            Petr Skoda added a comment - +1 for he patch (master only)
            Hide
            Rajesh Taneja added a comment -

            Thanks for the review Petr,

            Pushing it for integration.

            Show
            Rajesh Taneja added a comment - Thanks for the review Petr, Pushing it for integration.
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Integrated (master only), thanks!

            Show
            Eloy Lafuente (stronk7) added a comment - Integrated (master only), thanks!
            Hide
            Frédéric Massart added a comment -

            Test passed. Thanks!

            Show
            Frédéric Massart added a comment - Test passed. Thanks!
            Hide
            Eloy Lafuente (stronk7) added a comment -

            This issue has been integrated upstream and is now available both via git and cvs (and in some hours, via mirrors and downloads).

            Thanks!

            PS: Yay, legacy template messages. Yes, you're ok, we don't have CVS anymore!

            Show
            Eloy Lafuente (stronk7) added a comment - This issue has been integrated upstream and is now available both via git and cvs (and in some hours, via mirrors and downloads). Thanks! PS: Yay, legacy template messages. Yes, you're ok, we don't have CVS anymore!

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved: