Moodle

Update seems to truncate minor revisions

Details

  • Type: Bug Bug
  • Status: Closed 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 from Repository:
  • Pull Master Branch:
    wip-mdl-38382
  • Pull Master Diff URL:

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

discovered while testing

Bug - A problem which impairs or prevents Moodle from functioning correctly. MDL-38378 DB Upgrade script contains hard coded database prefix.

  • Blocker - Blocks development and/or testing, prevents Moodle from running (applicable to bugs only)
  • Closed - The issue is considered finished, the resolution is correct. Issues which are closed should not be reopened.

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
Eloy Lafuente (stronk7) added a comment -

Aha, okey, agree, thanks!

Show
Eloy Lafuente (stronk7) added a comment - Aha, okey, agree, thanks!
Hide
Permalink
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
Permalink
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
Permalink
Petr Škoda added a comment -

+1 for he patch (master only)

Show
Petr Škoda added a comment - +1 for he patch (master only)
Hide
Permalink
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
Permalink
Eloy Lafuente (stronk7) added a comment -

Integrated (master only), thanks!

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

Test passed. Thanks!

Show
Frédéric Massart added a comment - Test passed. Thanks!
Hide
Permalink
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

Vote (0)
Watch (4)

Dates

  • Created:
    Updated:
    Resolved: