Details

    • Type: Sub-task
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.0
    • Fix Version/s: 2.0
    • Component/s: Portfolio
    • Labels:
      None
    • Affected Branches:
      MOODLE_20_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE

      Description

      The files /portfolio/*.php do not have require_login()
      Those scripts are using $USER and $SESSION, adding require logins and adding forgotten kill switch into portfolio/file.php

      Please review and test

        Gliffy Diagrams

          Activity

          Hide
          skodak Petr Skoda added a comment -

          silly question, why is add.php using optional_param() all over the place?

          It would be also nice if each script contained a short description describing what it does, especially when is uses something from session created by other scripts.

          Show
          skodak Petr Skoda added a comment - silly question, why is add.php using optional_param() all over the place? It would be also nice if each script contained a short description describing what it does, especially when is uses something from session created by other scripts.
          Hide
          mjollnir Penny Leach added a comment -

          Hey Petr, good catch. I'll add docs too.

          What do you want me to review? There's no patch on this bug and nothing in the version control tab?

          I don't understand your comment about optional_param - that isn't deprecated, what's the problem?

          Show
          mjollnir Penny Leach added a comment - Hey Petr, good catch. I'll add docs too. What do you want me to review? There's no patch on this bug and nothing in the version control tab? I don't understand your comment about optional_param - that isn't deprecated, what's the problem?
          Hide
          skodak Petr Skoda added a comment -

          ooops, I must have used some other bug number
          see cvs

          Show
          skodak Petr Skoda added a comment - ooops, I must have used some other bug number see cvs
          Hide
          mjollnir Penny Leach added a comment -

          looks fine.I'll look at the docs as time allows.

          Petr -still waiting on hearing back on the problem with optional_param.

          Show
          mjollnir Penny Leach added a comment - looks fine.I'll look at the docs as time allows. Petr -still waiting on hearing back on the problem with optional_param.
          Hide
          skodak Petr Skoda added a comment -

          from http://docs.moodle.org/en/Development:Coding:

          8. Wherever possible group all your required_param(), optional_param() and other variables initialisation at the beginning of each file to make them easy to find.

          Show
          skodak Petr Skoda added a comment - from http://docs.moodle.org/en/Development:Coding: 8. Wherever possible group all your required_param(), optional_param() and other variables initialisation at the beginning of each file to make them easy to find.
          Hide
          mjollnir Penny Leach added a comment -

          fair enough.

          Show
          mjollnir Penny Leach added a comment - fair enough.
          Hide
          skodak Petr Skoda added a comment -

          I suppose in this case could be separate groups at the start of each case/if

          Show
          skodak Petr Skoda added a comment - I suppose in this case could be separate groups at the start of each case/if
          Hide
          mjollnir Penny Leach added a comment -

          I think I was probably trying to avoid unnecessary variable assignment, but it really doesn't matter. I will just put it all at the top.

          Show
          mjollnir Penny Leach added a comment - I think I was probably trying to avoid unnecessary variable assignment, but it really doesn't matter. I will just put it all at the top.
          Hide
          mjollnir Penny Leach added a comment -

          added docs and moved all optional_param and required_param to the top.

          Show
          mjollnir Penny Leach added a comment - added docs and moved all optional_param and required_param to the top.

            People

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

              Dates

              • Created:
                Updated:
                Resolved:
                Fix Release Date:
                24/Nov/10