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

format_text should not rely on $PAGE global to get its context

    XMLWordPrintable

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.0
    • Fix Version/s: 2.0
    • Component/s: Libraries
    • Labels:
      None
    • Affected Branches:
      MOODLE_20_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE

      Description

      This potentially affects cron, mnet and ws.

      See comment in MDL-7336 and the following jabber chat transcript:

      15:57 < penny> can we talk about MDL-7336 now ?
      15:57 < moodlebot_> http://tracker.moodle.org/browse/MDL-7336 Allow filters to be
      enabled/disabled per course or activity - Assignee: Tim Hunt Type: New
      Feature Status: Resolved (Fixed) V:11 W:9
      15:58 < timhunt> Sure, but I don't have much to say.
      15:58 < penny> i'm happy to work around it by creating a fake $PAGE object but it strikes me
      that this is not really the best solution ...
      15:58 < timhunt> Question 1:
      15:59 < timhunt> at the moment, filtering depnds on $COURSE. It is reasonable to make it
      depend on current $PAGE->context (or however we communicate the context to
      format_string) instead?
      15:59 < timhunt> I think the ansewr to that is yes.
      15:59 < timhunt> So, the question is, what is the best way to pass the context in.
      15:59 < penny> well i agree if the case is that modules have different settings
      16:00 < timhunt> But, does anyone disagree with the above before I go on?
      16:00 < skodak> format_text should accept more params, because sometimes the context is
      different, I do not like much relying on the $PAGE->anything
      16:00 < penny> i would speculate that we remove the $course parameter, and replace it with
      $context and only use $PAGE->context if it's not there
      16:00 < timhunt> I have a better idea.
      16:01 < timhunt> I think we should create a new interface text_formatting_context, with
      methods like get_course, get_context, and whatever else format_string/_text
      need.
      16:01 < timhunt> Then make $PAGE implement that interface.
      16:02 < timhunt> But also make a simple class that just implements the interaface, and which
      can be used in places like cron.
      16:02 < penny> which gets passed as an argument ?
      16:02 < timhunt> Yes, gets passed as an argument.
      16:02 < penny> or is set up as a fake $PAGE ish object
      16:02 ! mattc [mattc@moodle.org] has left &developers []
      16:03 < penny> what if we have a get_text_formatting_context() method
      16:03 < penny> that looks for CRON or XMLRPC or whatever webservices defines
      16:03 < penny> and returns whatever those have setup, else returns $PAGE
      16:03 ! mattc [mattc@moodle.org] has joined &developers
      16:04 < penny> i guess maybe that can change over the lifecycle of a request within cron
      though ...
      16:04 < timhunt> Penny, yes.
      16:04 < penny> like it's not one per cron ...
      16:05 < skodak> eventually we will have to use $PAGE hacks in many areas I am afraid, cron
      is one such place
      16:05 < penny> i'm just worried about the paths through the code to get to where format_text
      is called
      16:05 < timhunt> Penny, yes.
      16:06 < timhunt> That is why it currently relies on global $COURSE.
      16:06 * penny nods
      16:06 < timhunt> It would be much better if it did not use any globals.
      16:06 < penny> so at the moment, we don't even really have an idea of what the ideal
      solution is ...
      16:06 < penny> is this code
      16:06 < penny> "finished" ?
      16:07 < penny> in the 2.0 sense
      16:07 < penny> ?
      16:07 < timhunt> An interface that encapsulates text_formatting_context would also make it
      much easier to konw what should be used in the key of the text cache.
      16:07 < penny> i mean i seem to be the only one having problems with it ...
      16:07 < timhunt> Well, when I added the context bit, I was assuming that the format changes
      would clean up the format_text API, but I was wrong about that.
      16:08 < timhunt> Penny, I bet no-one had tested forum cron yet, and that will also be a
      killer.
      16:08 < penny> i was also wondering about webservices
      16:09 < penny> which surely is another problem about paths through code... if theoretically
      you can add any method to be callable over ws ...
      16:09 < penny> cron is more of a "devil you know" type problem
      16:11 < penny> who is the current maintainer of the format stuff
      16:11 < penny> i'm going to open a bug for this anyway
      16:12 < penny> no idea who to assign it to though
      16:12 < skodak> text formatting is not finished yet, I would like to work on that soon - we
      need it to support wiki syntax everywhre
      16:12 < penny> ok i'll assign it to you then

        Attachments

          Issue Links

            Activity

              People

              • Votes:
                1 Vote for this issue
                Watchers:
                4 Start watching this issue

                Dates

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