Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Won't Fix
    • Affects Version/s: 1.9
    • Fix Version/s: None
    • Component/s: Libraries
    • Labels:
      None
    • Affected Branches:
      MOODLE_19_STABLE
    • Rank:
      4195

      Description

      The log table url field is 100 characters. This can be a bit limiting in some instances where log entries may include text strings, such as filenames. (The easiest way to cause a problem with this is to make a wiki page with a 100-character name.)

      There are three related problems:

      1) Calling add_to_log with something longer than the url field results in an SQL error, and sends an email to the admin suggesting the database might have run out of disk space. While this behaviour is pretty cool, I don't think 'some stupid module tried to log a really long url' is serious enough for an admin email My suggested fix is:

      a) Check length is under the limited for 'url' and 'info' parameters (only ones which are likely to potentially depend on user input) and trim to limited length, with '...', if needed.

      b) If this trim happens, show debugging() info so that developers might notice they are logging something that can get too long.

      c) In log view, if url field ends with ..., don't treat it as a link (as it won't work!).

      2) The phpdoc for add_to_log is unclear as to the value of the URL field. It might seem obvious for developers to assume that it is, in fact, a fully qualified URL; however in fact it doesn't have to be, and the precise value for it depends on the type of plugin (sigh). It's difficult to track this down without following through a variety of code in log view, and when you do find out how it works, it's complicated.

      a) Change make_log_url in course/lib.php so that it supports block_, format_ prefixes in $module (if you passed in block_whatever and url frog.php at present, it would change the URL to /mod/block_whatever/frog.php - not helpful) and so that [apart from the existing exceptions] it is possible to begin your URL with / to mean 'start at $CFG->wwwroot' whatever the $module. The intent would be to make this function return exactly the same as it does now for all uses of the current function that return a correct url, but make it more consistent for future use and support blocks, course formats.

      b) Make phpdoc for add_to_log reflect this.

      3) 100 characters is too short in some cases especially if full URLs are logged. (are they supposed to be? maybe it's relative to $CFG->wwwroot, but this isn't clear in the phpdoc, this should also be clarified). I think the limit should be increased to 255. Could this cause problems for some databases, or is that change ok?

      I haven't done the code for this yet, if it is ok I will do it for moodle 1.9.1 or just for 2.0, let me know which. (I'll remember to mark only the relevant version fixed as per martin's request in developer meeting!)

        Issue Links

        Progress
        Resolved Sub-Tasks

        Sub-Tasks

        There are no Sub-Tasks for this issue.

          Activity

          Hide
          Eloy Lafuente (stronk7) added a comment -

          Hi Sam, some comments about your cool ideas:

          Point 1:
          ======

          Agree 100% in all comments: A, B & C. Sounds like an interesting prevention + debugging (also if we decide to expand the field to 25cc, same behaviour continues being valid IMO).

          +1 for 19_STABLE.

          Point 2:
          ======
          phpdoc for add_to_log must reflect whatever we decide about the url field, 100% agree. And IMO it should be ALWAYS the relative URL within the module/block... whatever. And never full URLs (I think we have some awful exceptions right now, like the "upload" action) nor "foreign" URLs.

          Now the question is, what are the possibilities?

          a) Keep current use, implementing the prefix_xxxx hack (in order to support blocks and other plugins logging as suggested by you).
          b) Add information to logs, but splitting the type of module (mod, block, filter...) and the subtype (forum...).

          Do we think a) is safe enough? Can we rely on those prefixes always? It's the easier solution. Just need to be 100% sure it'll cover all the cases.

          In fact, to avoid errors, I think it's also highly possible to automatically detect the proper module type and subtype calling the add_to_log() function, just based in the request. Perhaps we could make those parameters optional (for exceptions), relying in automatic detection for 95% of situations. Just one idea.

          But perhaps I'd delay this to 2.0. Have to analyse carefully how that change affects the upgrade in BIG sites (note that both solutions require DB upgrade, to add one new field or to make bigger the "module" one. Perhaps too much for one stable branch.

          +1 for 2.0

          Point 3:
          ======
          The change from of url from 100cc to 255 is ok for 2.0. I really think we need it.There are some pages like the wiki links... that can easily exceed the 100cc limit if we want to use URLs with the name of the pages (or perhaps we should be sticky about use always ids).

          But once more, it requires altering the logs table and that must be measured under all DBs IMO.

          +1 for 2.0

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Hi Sam, some comments about your cool ideas: Point 1: ====== Agree 100% in all comments: A, B & C. Sounds like an interesting prevention + debugging (also if we decide to expand the field to 25cc, same behaviour continues being valid IMO). +1 for 19_STABLE. Point 2: ====== phpdoc for add_to_log must reflect whatever we decide about the url field, 100% agree. And IMO it should be ALWAYS the relative URL within the module/block... whatever. And never full URLs (I think we have some awful exceptions right now, like the "upload" action) nor "foreign" URLs. Now the question is, what are the possibilities? a) Keep current use, implementing the prefix_xxxx hack (in order to support blocks and other plugins logging as suggested by you). b) Add information to logs, but splitting the type of module (mod, block, filter...) and the subtype (forum...). Do we think a) is safe enough? Can we rely on those prefixes always? It's the easier solution. Just need to be 100% sure it'll cover all the cases. In fact, to avoid errors, I think it's also highly possible to automatically detect the proper module type and subtype calling the add_to_log() function, just based in the request. Perhaps we could make those parameters optional (for exceptions), relying in automatic detection for 95% of situations. Just one idea. But perhaps I'd delay this to 2.0. Have to analyse carefully how that change affects the upgrade in BIG sites (note that both solutions require DB upgrade, to add one new field or to make bigger the "module" one. Perhaps too much for one stable branch. +1 for 2.0 Point 3: ====== The change from of url from 100cc to 255 is ok for 2.0. I really think we need it.There are some pages like the wiki links... that can easily exceed the 100cc limit if we want to use URLs with the name of the pages (or perhaps we should be sticky about use always ids). But once more, it requires altering the logs table and that must be measured under all DBs IMO. +1 for 2.0 Ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Feel free to split this in subtasks if that helps to have ideas organized.

          Show
          Eloy Lafuente (stronk7) added a comment - Feel free to split this in subtasks if that helps to have ideas organized.
          Hide
          Michael de Raadt added a comment -

          Thanks for reporting this issue.

          We have detected that this issue has been inactive for over a year has been recorded as affecting versions that are no longer supported.

          If you believe that this issue is still relevant to current versions (2.3 and beyond), please comment on the issue. Issues left inactive for a further month will be closed.

          Michael d;

          4d6f6f646c6521

          Show
          Michael de Raadt added a comment - Thanks for reporting this issue. We have detected that this issue has been inactive for over a year has been recorded as affecting versions that are no longer supported. If you believe that this issue is still relevant to current versions (2.3 and beyond), please comment on the issue. Issues left inactive for a further month will be closed. Michael d; 4d6f6f646c6521
          Hide
          Michael de Raadt added a comment -

          I'm closing this issue as it has been inactive for over a year has been recorded as affecting versions that are no longer supported.

          If you still believe this is an issue in supported versions, please report a new issue.

          Show
          Michael de Raadt added a comment - I'm closing this issue as it has been inactive for over a year has been recorded as affecting versions that are no longer supported. If you still believe this is an issue in supported versions, please report a new issue.

            People

            • Votes:
              2 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: