Moodle
  1. Moodle
  2. MDL-34035

A way to have more help links relative to wwwroot

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3
    • Fix Version/s: 2.3.1
    • Component/s: Language
    • Labels:
      None
    • Testing Instructions:
      Hide

      1. Test some help icons on your favourite Moodle forms, to make sure this does not cause any regressions.

      2. I have already tested the new features using qtype_stack.

      Show
      1. Test some help icons on your favourite Moodle forms, to make sure this does not cause any regressions. 2. I have already tested the new features using qtype_stack.
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
    • Rank:
      42330

      Description

      For a plugin I am working on (https://github.com/maths/moodle-qtype_stack) we would like the 'More help' link in help pop-ups to go to a script within out plugin (e.g. $CFG->wwwroot . '/question/type/stack/doc/doc.php/Authoring/Potential_response_trees.md#Answer_note') rather than to Moodle docs.

      This has two advantages:
      1. It lets us manage the help content in the way we prefer.
      2. The 'More help' can include PHP-generated content, like the Emoticons help used to do in Moodle 1.9.

      At the same time, we should add support for absolute URLs (starting http(s)://) to cover the use cases where people want to link to their own online help.

        Activity

        Hide
        Tim Hunt added a comment -

        Adding some people who might object to this, and submitting for peer review.

        Show
        Tim Hunt added a comment - Adding some people who might object to this, and submitting for peer review.
        Hide
        David Mudrak added a comment -

        Thanks for the patch Tim. The only part that caught my eye was the https://github.com/timhunt/moodle/compare/master...MDL-34035#L0L595 but it seems to be reasonable (and actually very useful).

        Show
        David Mudrak added a comment - Thanks for the patch Tim. The only part that caught my eye was the https://github.com/timhunt/moodle/compare/master...MDL-34035#L0L595 but it seems to be reasonable (and actually very useful).
        Hide
        Tim Hunt added a comment -

        I could not tell if the hard-coding of en on that line was because current_lang() does not work during a new install, or something like that, or if it was just a mistake. I need to test a new install. I think if the hard coded en is necessary, then we should document it with a comment.

        Since there is no integration this week, I will leave it until the end of the week for people to comment, before I submit it for integration.

        Because we need this for qtype_stack, I would really like to get it added to 2.3+, even though technically it is a new feature. (We want STACK to be compatible with 2.3.x). The change is backwards compatible, so I don't think there is a risk adding it to stable branches.

        Show
        Tim Hunt added a comment - I could not tell if the hard-coding of en on that line was because current_lang() does not work during a new install, or something like that, or if it was just a mistake. I need to test a new install. I think if the hard coded en is necessary, then we should document it with a comment. Since there is no integration this week, I will leave it until the end of the week for people to comment, before I submit it for integration. Because we need this for qtype_stack, I would really like to get it added to 2.3+, even though technically it is a new feature. (We want STACK to be compatible with 2.3.x). The change is backwards compatible, so I don't think there is a risk adding it to stable branches.
        Hide
        Tim Hunt added a comment -

        I hear no objections, so I am submitting this for integration.

        I am not sure if this should be cherry-picked to stable branches. As I say, I am happy for the integrators to choose, but please can I ask for it to go into 2.3.x for the benefit of STACK.

        Show
        Tim Hunt added a comment - I hear no objections, so I am submitting this for integration. I am not sure if this should be cherry-picked to stable branches. As I say, I am happy for the integrators to choose, but please can I ask for it to go into 2.3.x for the benefit of STACK.
        Hide
        Dan Poltawski added a comment -

        At first when reviewing this I thought I was missing a piece of the puzzle because this doesn't feel very natural change at all to me.

        The change is making get_docs_url - a function which its main business is generating a url based on version/language/docsroot change behaviour to return a url which isn't influenced by any of these variables. It seemed like a complete repurposing of the function to me.

        Furthermore it begged the question why calling code doesn't just generate the url using moodle_url itself rather than calling get_docs_url() to do this. Eventually it dawned on me why you were looking for this behaviour - I think its because the path is specified in a lang string. But even knowing this, it still doesn't feel like the right place for this functionality.

        It seems like it'd make more sense to place this logic into the doc_link function? But maybe i'm missing something?

        Show
        Dan Poltawski added a comment - At first when reviewing this I thought I was missing a piece of the puzzle because this doesn't feel very natural change at all to me. The change is making get_docs_url - a function which its main business is generating a url based on version/language/docsroot change behaviour to return a url which isn't influenced by any of these variables. It seemed like a complete repurposing of the function to me. Furthermore it begged the question why calling code doesn't just generate the url using moodle_url itself rather than calling get_docs_url() to do this. Eventually it dawned on me why you were looking for this behaviour - I think its because the path is specified in a lang string. But even knowing this, it still doesn't feel like the right place for this functionality. It seems like it'd make more sense to place this logic into the doc_link function? But maybe i'm missing something?
        Hide
        David Mudrak added a comment -

        Dan, see https://github.com/moodle/moodle/blob/master/help.php#L74 This is where we generate the link to the docs for the help popup. Until now, we expected that the docs always live in our MediaWiki. But for contrib plugins, this just seems too limiting - they may want to link their own site, github wiki or whatever (including Tim's case of the docs actually shipped with the code itself). So in my eyes, Tim's patch just extends the current scope of the get_docs_url(), it does not repurpose it.

        Show
        David Mudrak added a comment - Dan, see https://github.com/moodle/moodle/blob/master/help.php#L74 This is where we generate the link to the docs for the help popup. Until now, we expected that the docs always live in our MediaWiki. But for contrib plugins, this just seems too limiting - they may want to link their own site, github wiki or whatever (including Tim's case of the docs actually shipped with the code itself). So in my eyes, Tim's patch just extends the current scope of the get_docs_url(), it does not repurpose it.
        Hide
        Tim Hunt added a comment -

        Dan when you say "more sense to place this logic into the doc_link function?" are you talking about $OUTPUT->doc_link()?

        If you are, then you are just wrong. Renderer functions should be about generating teh HTML to display something, not logic. Taking the contents of a lang string like somehelptopic_link, and making a URL is logic, not display.

        Therefore, the library function get_docs_url() is reasonable place for this.

        Show
        Tim Hunt added a comment - Dan when you say "more sense to place this logic into the doc_link function?" are you talking about $OUTPUT->doc_link()? If you are, then you are just wrong. Renderer functions should be about generating teh HTML to display something, not logic. Taking the contents of a lang string like somehelptopic_link, and making a URL is logic, not display. Therefore, the library function get_docs_url() is reasonable place for this.
        Hide
        Sam Hemelryk added a comment -

        Hi all,

        I had a quick look at this issue myself.
        Having looked over the changes here and worked my way through how everything ties together, I think the purposed changes are fine.
        At first I was a little confused as to why it was now possible pass a URL through get_docs_url but in looking into how everything was being chained together I can see why you've implemented it that way. Certainly at the moment that is the best place for these changes.
        Gets a +1 from me.
        While looking I did note a couple of really trivial little things, don't need fixing really but you always could if you wanted.

        1. There's a typo in the phpdocs "2. $path may be an absolute URL, starting http:// or http://"
        2. It'd be better to use strpos rather than substr for the token searches, strpos is less memory intensive and slightly faster.... of course one call per page on ave so not exactly anything you'd ever notice.

        Cheers
        Sam

        Show
        Sam Hemelryk added a comment - Hi all, I had a quick look at this issue myself. Having looked over the changes here and worked my way through how everything ties together, I think the purposed changes are fine. At first I was a little confused as to why it was now possible pass a URL through get_docs_url but in looking into how everything was being chained together I can see why you've implemented it that way. Certainly at the moment that is the best place for these changes. Gets a +1 from me. While looking I did note a couple of really trivial little things, don't need fixing really but you always could if you wanted. There's a typo in the phpdocs "2. $path may be an absolute URL, starting http:// or http://" It'd be better to use strpos rather than substr for the token searches, strpos is less memory intensive and slightly faster.... of course one call per page on ave so not exactly anything you'd ever notice. Cheers Sam
        Hide
        Dan Poltawski added a comment -

        I take your points about doc_link (i'm wasn't proposing to add logic into the renderer, I had actually missed that detail) - the change just looked unusual to me as commented. Looking more I agree the location of the change makes sense.

        I've integrated this now to 23 and master.

        In order to speed up the process I did one commit to fix the the https typo Sam spotted. We can change the strpos in future in a new issue if we care enough. (And it should be straight forward to change since we have unit tests now )

        Show
        Dan Poltawski added a comment - I take your points about doc_link (i'm wasn't proposing to add logic into the renderer, I had actually missed that detail) - the change just looked unusual to me as commented. Looking more I agree the location of the change makes sense. I've integrated this now to 23 and master. In order to speed up the process I did one commit to fix the the https typo Sam spotted. We can change the strpos in future in a new issue if we care enough. (And it should be straight forward to change since we have unit tests now )
        Hide
        Tim Hunt added a comment -

        Thanks Dan and Sam.

        I am not convinced that strpos is necessarily faster than substr. Think about the case where $path is 1GB (which will never happen here). Really, PHP needs a 'str_starts_with' method. Actually, there is one strncmp($path, 'http://', 7) == 0. Doh!

        Show
        Tim Hunt added a comment - Thanks Dan and Sam. I am not convinced that strpos is necessarily faster than substr. Think about the case where $path is 1GB (which will never happen here). Really, PHP needs a 'str_starts_with' method. Actually, there is one strncmp($path, 'http://', 7) == 0. Doh!
        Hide
        Sam Hemelryk added a comment -

        Congratulations your code is upstream - gold star for you!

        This issue + 79 others made it in in time for the minor releases.
        Thank you everyone involved for your exuberant efforts.

        Show
        Sam Hemelryk added a comment - Congratulations your code is upstream - gold star for you! This issue + 79 others made it in in time for the minor releases. Thank you everyone involved for your exuberant efforts.
        Hide
        Dan Poltawski added a comment -

        Michael has pointed out that this hasn't been documented, or has the docs_required flag.

        Tim - could you document this new feature?

        Show
        Dan Poltawski added a comment - Michael has pointed out that this hasn't been documented, or has the docs_required flag. Tim - could you document this new feature?
        Hide
        Michael de Raadt added a comment -

        I should have contributed to this earlier.

        Tim: Can you add something about this in the Dev docs so that other can make use of it?

        This should probably have been held in Master until 2.4.

        It would have been good if this issue was labelled with at least "ui_change" so it could have been added to the release notes. I've retrospectively add it to http://docs.moodle.org/dev/Moodle_2.3.1_release_notes

        Show
        Michael de Raadt added a comment - I should have contributed to this earlier. Tim: Can you add something about this in the Dev docs so that other can make use of it? This should probably have been held in Master until 2.4. It would have been good if this issue was labelled with at least "ui_change" so it could have been added to the release notes. I've retrospectively add it to http://docs.moodle.org/dev/Moodle_2.3.1_release_notes
        Hide
        Dan Poltawski added a comment -

        Michael: I don't agree that it should've held off (hence integrating).

        It was low risk, came with unit tests and helps a significant contrib project.

        Show
        Dan Poltawski added a comment - Michael: I don't agree that it should've held off (hence integrating). It was low risk, came with unit tests and helps a significant contrib project.
        Hide
        David Mudrak added a comment -
        • Why should this be labelled with "ui_change"? There is no actual UI change introduced by this patch
        • Yes, generally things like this should go to master. However, Tim had very good reasons and motivation to have it included in 2.3 and did a very good job preparing the patch. Hence it was fair to make an exception and attribute this community input.
        • I just added http://docs.moodle.org/dev/Help_strings#Links_to_additional_documentation section. Please review it and remove the docs_required label from this issue. TIA.
        Show
        David Mudrak added a comment - Why should this be labelled with "ui_change"? There is no actual UI change introduced by this patch Yes, generally things like this should go to master. However, Tim had very good reasons and motivation to have it included in 2.3 and did a very good job preparing the patch. Hence it was fair to make an exception and attribute this community input. I just added http://docs.moodle.org/dev/Help_strings#Links_to_additional_documentation section. Please review it and remove the docs_required label from this issue. TIA.
        Hide
        Tim Hunt added a comment -

        Of course, I could not edit dev docs until I knew this was going to be integrated.

        Docs now added to http://docs.moodle.org/dev/Help_strings. That page seems to duplicate http://docs.moodle.org/dev/String_API a lot. Someone should refactor that help.

        Like Dan, I see no reason to keep this out of stable branches. Give me one even vaguely plausible scenario where this would cause a regression? How is this a UI change? Name one bit of standard Moodle functionality that has changed here, and which will require people to update their documentation? See, there aren't any.

        Anyway, I made you a watcher early on, and you had plenty of opportunity to raise your objection before integration.

        Show
        Tim Hunt added a comment - Of course, I could not edit dev docs until I knew this was going to be integrated. Docs now added to http://docs.moodle.org/dev/Help_strings . That page seems to duplicate http://docs.moodle.org/dev/String_API a lot. Someone should refactor that help. Like Dan, I see no reason to keep this out of stable branches. Give me one even vaguely plausible scenario where this would cause a regression? How is this a UI change? Name one bit of standard Moodle functionality that has changed here, and which will require people to update their documentation? See, there aren't any. Anyway, I made you a watcher early on, and you had plenty of opportunity to raise your objection before integration.
        Hide
        David Mudrak added a comment -

        LOL I just realized Tim added the documentation simultaneously. So now we have extra work to merge those two edits

        Show
        David Mudrak added a comment - LOL I just realized Tim added the documentation simultaneously. So now we have extra work to merge those two edits
        Hide
        Tim Hunt added a comment -

        Oh woops! a feature so good they documented it twice. Thanks David. Now, which of us shall clean up the mess, or shall we now delete both lots of docs

        Show
        Tim Hunt added a comment - Oh woops! a feature so good they documented it twice. Thanks David. Now, which of us shall clean up the mess, or shall we now delete both lots of docs
        Hide
        David Mudrak added a comment -

        I'll do it, thanks.

        Show
        David Mudrak added a comment - I'll do it, thanks.
        Hide
        David Mudrak added a comment -
        Show
        David Mudrak added a comment - Done, the merged version is at http://docs.moodle.org/dev/Help_strings#More_help_links
        Hide
        Tim Hunt added a comment -

        Thanks David. Looks good to me.

        Show
        Tim Hunt added a comment - Thanks David. Looks good to me.
        Hide
        Michael de Raadt added a comment -

        Thanks for working on this, guys.

        David: While it's more of an API change than a UI change, we don't use an "api_change" label. Perhaps we should, but the effect would be the same - to get the issue noted in the release notes.

        Show
        Michael de Raadt added a comment - Thanks for working on this, guys. David: While it's more of an API change than a UI change, we don't use an "api_change" label. Perhaps we should, but the effect would be the same - to get the issue noted in the release notes.
        Hide
        David Mudrak added a comment -

        Hmm, I really don't think that abusing labels in this way is a good idea. If you need a way to remember issues to be mentioned in release notes, better to create release_notes label or so. Using other labels just for that purpose is just confusing for other people using this tracker.

        Show
        David Mudrak added a comment - Hmm, I really don't think that abusing labels in this way is a good idea. If you need a way to remember issues to be mentioned in release notes, better to create release_notes label or so. Using other labels just for that purpose is just confusing for other people using this tracker.
        Hide
        Michael de Raadt added a comment -

        Hi, David.

        This is not an abuse of the ui_change label. It is the purpose of the label and the reason I brought it into existence.

        You may be right about the name, but it's not easy to change now.

        Show
        Michael de Raadt added a comment - Hi, David. This is not an abuse of the ui_change label. It is the purpose of the label and the reason I brought it into existence. You may be right about the name, but it's not easy to change now.
        Hide
        Tim Hunt added a comment -

        I think it is better to have separate labels like api_change, ui_change to record the different types of information that need to be included in the release notes, rather than a single generic release_notes label.

        Show
        Tim Hunt added a comment - I think it is better to have separate labels like api_change, ui_change to record the different types of information that need to be included in the release notes, rather than a single generic release_notes label.
        Hide
        Helen Foster added a comment -

        Let's just make sure that all the labels we use are described in http://docs.moodle.org/dev/Tracker_issue_labels so everyone can understand their purposes.

        Show
        Helen Foster added a comment - Let's just make sure that all the labels we use are described in http://docs.moodle.org/dev/Tracker_issue_labels so everyone can understand their purposes.
        Hide
        Michael de Raadt added a comment -

        Just noting that we now have an api_change label and it is documented in http://docs.moodle.org/dev/Tracker_issue_labels.

        Show
        Michael de Raadt added a comment - Just noting that we now have an api_change label and it is documented in http://docs.moodle.org/dev/Tracker_issue_labels .

          People

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

            Dates

            • Created:
              Updated:
              Resolved: