Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-40200

PHP Notice when viewing the profile of a not existing user

          Details

          • Type: Bug
          • Status: Closed
          • Priority: Minor
          • Resolution: Fixed
          • Affects Version/s: 2.3.7, 2.4.4, 2.5, 2.6
          • Fix Version/s: 2.3.8, 2.4.5, 2.5.1
          • Component/s: Administration
          • Labels:
          • Testing Instructions:
            Hide

            (Difficulty: easy, but needs testing against all branches)

            1. As a Moodle Administrator, point the browser to a profile URL where the userid is out of the range of the registered users, e.g. http://hostname/path/moodle/user/profile.php?id=1370780391: a message (not an exception) saying Invalid user will be printed out;
            2. As a Moodle Administrator, point the browser to a profile URL related to a deleted user: a message saying This user account has been deleted will be printed out.
            Show
            (Difficulty: easy, but needs testing against all branches) As a Moodle Administrator, point the browser to a profile URL where the userid is out of the range of the registered users, e.g. http://hostname/path/moodle/user/profile.php?id=1370780391: a message (not an exception) saying Invalid user will be printed out; As a Moodle Administrator, point the browser to a profile URL related to a deleted user: a message saying This user account has been deleted will be printed out.
          • Affected Branches:
            MOODLE_23_STABLE, MOODLE_24_STABLE, MOODLE_25_STABLE, MOODLE_26_STABLE
          • Fixed Branches:
            MOODLE_23_STABLE, MOODLE_24_STABLE, MOODLE_25_STABLE
          • Pull from Repository:
          • Pull Master Branch:
            m26_MDL-40200_Notices_When_Viewing_Profile_Invalid_UserId
          • Pull Master Diff URL:

            Description

            While working on MDL-39810, I found a trivial issue in profile.php triggered by using userid not in the database: an exception is thrown (invalid user) but two notices are added two at the end of the message.

              Gliffy Diagrams

                Attachments

                  Issue Links

                  has a non-specific relationship to

                  Improvement - An enhancement to an existing Moodle feature. MDL-30407 get_record MUST_EXIST should not throw dml_exception

                  • Minor - Minor loss of function where workaround is possible
                  • Open
                  Testing discovered

                  Bug - A problem which impairs or prevents Moodle from functioning correctly. MDL-39810 IE Compatibility View problem - Moodle 2.5 repository_ajax.php tries to download in browser

                  • Major - Major loss of function, incorrect output
                  • Closed

                    Activity

                    Ascending order - Click to sort in descending order
                    Hide
                    Permalink
                    matteo Matteo Scaramuccia added a comment -

                    Hi Rajesh,
                    could you make a peer review of this trivial issue? Lowest priority, being almost code cleanup.
                    I'm available to prepare the backport to other branches too, if required and as preferred.

                    Show
                    matteo Matteo Scaramuccia added a comment - Hi Rajesh, could you make a peer review of this trivial issue? Lowest priority, being almost code cleanup. I'm available to prepare the backport to other branches too, if required and as preferred.
                    Hide
                    Permalink
                    salvetore Michael de Raadt added a comment -

                    Thanks for working on that, Matteo

                    Show
                    salvetore Michael de Raadt added a comment - Thanks for working on that, Matteo
                    Hide
                    Permalink
                    rajeshtaneja Rajesh Taneja added a comment -

                    Thanks Matteo for reporting this issue and providing a fix.

                    Patch looks good and works great.
                    My only concern is adding duplicate code. Should this be combined with logic of $user->deleted check?
                    I would have considered passing MUST_EXIST to get_record and with MDL-30407 fix it will be moodle_exception and not dml_exception, but looking at MDL-30407 status, it seems going with notification is better for now.

                    Show
                    rajeshtaneja Rajesh Taneja added a comment - Thanks Matteo for reporting this issue and providing a fix. Patch looks good and works great. My only concern is adding duplicate code. Should this be combined with logic of $user->deleted check? I would have considered passing MUST_EXIST to get_record and with MDL-30407 fix it will be moodle_exception and not dml_exception, but looking at MDL-30407 status, it seems going with notification is better for now.
                    Hide
                    Permalink
                    matteo Matteo Scaramuccia added a comment -

                    Hi Rajesh,
                    I'll look at a way to improve it.

                    Show
                    matteo Matteo Scaramuccia added a comment - Hi Rajesh, I'll look at a way to improve it.
                    Hide
                    Permalink
                    matteo Matteo Scaramuccia added a comment -

                    Hi Rajesh,
                    avoiding code duplication get the conditions less readable: if you're happy with that, I'll "fixup" the two commit into a single one.

                    Show
                    matteo Matteo Scaramuccia added a comment - Hi Rajesh, avoiding code duplication get the conditions less readable: if you're happy with that, I'll " fixup " the two commit into a single one.
                    Hide
                    Permalink
                    rajeshtaneja Rajesh Taneja added a comment -

                    Thanks for fixing this Matteo,

                    Patch looks great, pushing it for integration.

                    Show
                    rajeshtaneja Rajesh Taneja added a comment - Thanks for fixing this Matteo, Patch looks great, pushing it for integration.
                    Hide
                    Permalink
                    poltawski Dan Poltawski added a comment -

                    The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

                    TIA and ciao

                    Show
                    poltawski Dan Poltawski added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
                    Hide
                    Permalink
                    matteo Matteo Scaramuccia added a comment -

                    Squashed, rebased, added 2.5 branch.

                    Show
                    matteo Matteo Scaramuccia added a comment - Squashed, rebased, added 2.5 branch.
                    Hide
                    Permalink
                    stronk7 Eloy Lafuente (stronk7) added a comment -

                    Integrated (23, 24, 25 & master), thanks!

                    To tested, plz, test this under all branches, there was some conflict resolution in 23_STABLE.

                    Ciao

                    Show
                    stronk7 Eloy Lafuente (stronk7) added a comment - Integrated (23, 24, 25 & master), thanks! To tested, plz, test this under all branches, there was some conflict resolution in 23_STABLE. Ciao
                    Hide
                    Permalink
                    stronk7 Eloy Lafuente (stronk7) added a comment -

                    (Offtopic consideration: Some day we should start unifying all the error/notifications). They are shown in a really different/inconsistent way).

                    Show
                    stronk7 Eloy Lafuente (stronk7) added a comment - (Offtopic consideration: Some day we should start unifying all the error/notifications). They are shown in a really different/inconsistent way).
                    Hide
                    Permalink
                    samhemelryk Sam Hemelryk added a comment -

                    Tested and passed thanks guys.

                    Show
                    samhemelryk Sam Hemelryk added a comment - Tested and passed thanks guys.
                    Hide
                    Permalink
                    stronk7 Eloy Lafuente (stronk7) added a comment -

                    Thanks for giving me joys and smiles
                    Thanks for sharing my trouble's pile

                    Thanks for wipeing the tears of my eye
                    Thanks for showing me the glad view of sky

                    Thanks for lending me your shoulders to lean
                    Thanks for giving my words a proper mean

                    Thanks for telling me the value of life
                    Thanks for showing me the rules to survive

                    Thanks for lending me the sympathetic ears
                    Thanks for showing how much you care

                    From all this what I mean in the end
                    Is thanks for being my special friend.

                    – Seema Chowdhury

                    Sent upstream so... closing, thanks!

                    Show
                    stronk7 Eloy Lafuente (stronk7) added a comment - Thanks for giving me joys and smiles Thanks for sharing my trouble's pile Thanks for wipeing the tears of my eye Thanks for showing me the glad view of sky Thanks for lending me your shoulders to lean Thanks for giving my words a proper mean Thanks for telling me the value of life Thanks for showing me the rules to survive Thanks for lending me the sympathetic ears Thanks for showing how much you care From all this what I mean in the end Is thanks for being my special friend. – Seema Chowdhury Sent upstream so... closing, thanks!

                      People

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

                        Dates

                        • Created:
                          Updated:
                          Resolved:
                          Fix Release Date:
                          8/Jul/13