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
    • Rank:
      48291

      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.

        Issue Links

          Activity

          Hide
          Petr Škoda added a comment -

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

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

          Dear Petr Škoda,

          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 Škoda , 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 Škoda added a comment -

          +1 for he patch (master only)

          Show
          Petr Škoda 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: