Moodle
  1. Moodle
  2. MDL-28044

doc links generated by print_error() needs to be updated to point to new versioned docs url

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 1.9.12
    • Fix Version/s: 1.9.13, 2.0.4, 2.1
    • Component/s: General
    • Labels:
      None
    • Testing Instructions:
      Hide

      1) create any step that shows an error page (like the one in described in description) , link should be relevant and not broken.
      2) test install of 19_STABLE
      3) test upgrade from 19_STABLE TO master

      Show
      1) create any step that shows an error page (like the one in described in description) , link should be relevant and not broken. 2) test install of 19_STABLE 3) test upgrade from 19_STABLE TO master
    • Affected Branches:
      MOODLE_19_STABLE
    • Fixed Branches:
      MOODLE_19_STABLE, MOODLE_20_STABLE, MOODLE_21_STABLE
    • Pull from Repository:
    • Pull Master Branch:
    • Rank:
      17725

      Description

      From Aaron Wells:
      "...
      doc links generated by print_error().

      For instance, in Moodle 1.9 if I try to access admin/index.php while logged in as a student, I'll get an error page with a little link that reads "More information about this error". The link leads to "http://docs.moodle.org/en/error/moodle/nopermissions", which of course redirects to the Moodle 2.0 documentation."

        Issue Links

          Activity

          Hide
          Aparup Banerjee added a comment -

          SamH, whats your take on this fix?

          Show
          Aparup Banerjee added a comment - SamH, whats your take on this fix?
          Hide
          Sam Hemelryk added a comment -

          Hi Apu,

          The changes look fine - but too be truthful I don't really like the way in which the function now serves two purposes either returning a fragment of HTML for a doc link or just the URL to the docs main page.

          Splitting it out is the obvious solution but given this is for Moodle 1.9 and has already be split in 1.9 I think that this sort of solution is fine.
          The only thing I don't then like about this new function is that the new argument leads to the previous 3 arguments being ignored.
          I really think if we are going to have this return type argument that it should return the docs url + path, basically you get the URL as an html fragment or you get just the URL. It would also involve renaming returndocroot to something like returnurlonly.
          I think if that is possible it would get a passive +1 from me.
          Otherwise a certain +1 to split the function and create a callable get_doc_root or something like that.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Apu, The changes look fine - but too be truthful I don't really like the way in which the function now serves two purposes either returning a fragment of HTML for a doc link or just the URL to the docs main page. Splitting it out is the obvious solution but given this is for Moodle 1.9 and has already be split in 1.9 I think that this sort of solution is fine. The only thing I don't then like about this new function is that the new argument leads to the previous 3 arguments being ignored. I really think if we are going to have this return type argument that it should return the docs url + path, basically you get the URL as an html fragment or you get just the URL. It would also involve renaming returndocroot to something like returnurlonly. I think if that is possible it would get a passive +1 from me. Otherwise a certain +1 to split the function and create a callable get_doc_root or something like that. Cheers Sam
          Hide
          Aparup Banerjee added a comment -

          Hi Sam,
          i went with splitting that bit up. i've updated the patch.

          Show
          Aparup Banerjee added a comment - Hi Sam, i went with splitting that bit up. i've updated the patch.
          Hide
          Sam Hemelryk added a comment -

          Hi Apu,

          Just been looking at this now - I like the new function, it all looks good, but there is one minor point.
          I think the new function get_doc_root should really be doing the if (empty($CFG->docroot)) { check that is being done by doc_link and doc_link should be checking the response of get_doc_root.
          Otherwise in your code at present if doc root isn't set and you get to an error page you will get a notice.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Apu, Just been looking at this now - I like the new function, it all looks good, but there is one minor point. I think the new function get_doc_root should really be doing the if (empty($CFG->docroot)) { check that is being done by doc_link and doc_link should be checking the response of get_doc_root. Otherwise in your code at present if doc root isn't set and you get to an error page you will get a notice. Cheers Sam
          Hide
          Aparup Banerjee added a comment -

          Sam,
          yup i've moved that check into the new function. (repo updated). there is still the case of when neither errordocroot nor docroot is set in print_error(). i've made that point to itself - which would be a dead end broke link. Is there a place to point to for configuration checks? is this too much development for 1.9 :-D ?

          Show
          Aparup Banerjee added a comment - Sam, yup i've moved that check into the new function. (repo updated). there is still the case of when neither errordocroot nor docroot is set in print_error(). i've made that point to itself - which would be a dead end broke link. Is there a place to point to for configuration checks? is this too much development for 1.9 :-D ?
          Hide
          Sam Hemelryk added a comment -

          Hi Apu,

          Looks good to me - I will just check with Eloy that he is happy for 1.9 changes to go in now or whether he'd like for it to wait until after 2.1 release (next week).

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Apu, Looks good to me - I will just check with Eloy that he is happy for 1.9 changes to go in now or whether he'd like for it to wait until after 2.1 release (next week). Cheers Sam
          Hide
          Aparup Banerjee added a comment -

          The 2.x ones are broke for print_error() .... get_exception_info() as well..
          the code/fix looks like it could be similar, doing one up now for the other branches.

          Show
          Aparup Banerjee added a comment - The 2.x ones are broke for print_error() .... get_exception_info() as well.. the code/fix looks like it could be similar, doing one up now for the other branches.
          Hide
          Aparup Banerjee added a comment -

          created patches for 2.x branches too for print_error() linking to new moodle docs.

          Show
          Aparup Banerjee added a comment - created patches for 2.x branches too for print_error() linking to new moodle docs.
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          Oh, sorry, but we shouldn't be including other libs in setuplib.php, it breaks the concept it was created to!

          So, there are 2 alternatives here:

          1) Move that code (handling $errordocroot) to later in the request process. (I think this CANNOT be done, it is the default exception handler!)
          2) Move all the required functions to lib/setuplib.php. So the get_docs_url() function will be moved from weblib to setuplib as far as earlier access to it is required. ( I think this could work, of course if that function has not other dependencies, I haven't looked to that!).

          But never include other libs to setuplib, it's nono.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Oh, sorry, but we shouldn't be including other libs in setuplib.php, it breaks the concept it was created to! So, there are 2 alternatives here: 1) Move that code (handling $errordocroot) to later in the request process. (I think this CANNOT be done, it is the default exception handler!) 2) Move all the required functions to lib/setuplib.php. So the get_docs_url() function will be moved from weblib to setuplib as far as earlier access to it is required. ( I think this could work, of course if that function has not other dependencies, I haven't looked to that!). But never include other libs to setuplib, it's nono. Ciao
          Hide
          Aparup Banerjee added a comment -

          ah so setuplib is a no dependency lib, thanks Eloy. yea get_docs_url() depends on just version.php (but loads it if not loaded).

          Show
          Aparup Banerjee added a comment - ah so setuplib is a no dependency lib, thanks Eloy. yea get_docs_url() depends on just version.php (but loads it if not loaded).
          Hide
          Aparup Banerjee added a comment -

          okay i've moved the function into setuplib.php , its up for integration review now.

          Show
          Aparup Banerjee added a comment - okay i've moved the function into setuplib.php , its up for integration review now.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          (added some tests I've not covered here, just to be sure the move does not affect install / upgrade)

          Show
          Eloy Lafuente (stronk7) added a comment - (added some tests I've not covered here, just to be sure the move does not affect install / upgrade)
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          Hi Aparup,

          I've integrated both the 20_STABLE and master branches. Perfect!

          But looking to the 19_STABLE branch... there are some bits to improve IMO:

          1) This condition is really awful syntactically (we use to stay away from assignments within conditions completely, more yet I don't know if that works as expected):

          if (is_null($errordoclink = get_doc_root())) {
          

          2) I don't understand why, if $CFG->docroot is empty, the "default" http://docs.moodle.org/19/$lang is not used (and you point to "self" instead).

          3) And the most I think on it... the less I like (this applies to all branches, not only 19_STABLE) the include of version.php at all nor the parsing of $release to calculate $branch. After all, each branch has different code and we could have some constant/define with the 19/20... That would make everything really easy IMO.

          So I'm keeping this open for now.. about the 19_STABLE problem and the version.php consideration. Just imagine we invent version 10.1 or whatever, that will break the regexp, while having it in some constant will do things really easier and free of problems loading version.php multiple times or so.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Hi Aparup, I've integrated both the 20_STABLE and master branches. Perfect! But looking to the 19_STABLE branch... there are some bits to improve IMO: 1) This condition is really awful syntactically (we use to stay away from assignments within conditions completely, more yet I don't know if that works as expected): if (is_null($errordoclink = get_doc_root())) { 2) I don't understand why, if $CFG->docroot is empty, the "default" http://docs.moodle.org/19/$lang is not used (and you point to "self" instead). 3) And the most I think on it... the less I like (this applies to all branches, not only 19_STABLE) the include of version.php at all nor the parsing of $release to calculate $branch. After all, each branch has different code and we could have some constant/define with the 19/20... That would make everything really easy IMO. So I'm keeping this open for now.. about the 19_STABLE problem and the version.php consideration. Just imagine we invent version 10.1 or whatever, that will break the regexp, while having it in some constant will do things really easier and free of problems loading version.php multiple times or so. Ciao
          Hide
          Aparup Banerjee added a comment - - edited

          Thanks Eloy, i'll talk to Sam about it soon and see where we go. for sure (1) & (2) is np to fix.

          Show
          Aparup Banerjee added a comment - - edited Thanks Eloy, i'll talk to Sam about it soon and see where we go. for sure (1) & (2) is np to fix.
          Hide
          Aparup Banerjee added a comment -

          i've updated my repo for 19 with (1) & (2)

          Show
          Aparup Banerjee added a comment - i've updated my repo for 19 with (1) & (2)
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Hi Aparup,

          I'm finally integrating the 1.9 branch but, the more I look at current behavior, the more I don't like it. Things like adding "en" or branch when custom URLs are used and also the way to fallback and the 3) above...

          Because of this, I've created this simpletest describing how the links should behave (at least in my mind). For master:

          https://github.com/stronk7/moodle/compare/master...doclinks_crazy_tests

          So, while this issue continues his cycle (testing...), I would recommend to

          • create a new MDL issue about to discuss this, check the unittest above and decide which is the preferred behavior (changing the tests if necessary, of course, current ones are only my POV). Also discuss the 3) above.
          • implement it, and push for integration.
          • Have beer

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Hi Aparup, I'm finally integrating the 1.9 branch but, the more I look at current behavior, the more I don't like it. Things like adding "en" or branch when custom URLs are used and also the way to fallback and the 3) above... Because of this, I've created this simpletest describing how the links should behave (at least in my mind). For master: https://github.com/stronk7/moodle/compare/master...doclinks_crazy_tests So, while this issue continues his cycle (testing...), I would recommend to create a new MDL issue about to discuss this, check the unittest above and decide which is the preferred behavior (changing the tests if necessary, of course, current ones are only my POV). Also discuss the 3) above. implement it, and push for integration. Have beer Ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integrated, thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated, thanks!
          Hide
          Aparup Banerjee added a comment -

          Hi Eloy,
          thats cool! i've created MDL-28134 with a variety of watchers for a good discussion
          Cheers!

          Show
          Aparup Banerjee added a comment - Hi Eloy, thats cool! i've created MDL-28134 with a variety of watchers for a good discussion Cheers!
          Hide
          Rajesh Taneja added a comment -

          Pass:
          1. All links point fine in 1.9.
          2. All links are fine in 2.0 except upgrade links.
          3. All links are fine in 2.1 except upgrade links.

          Fails:
          1. while upgrading from 1.9 to 2.0, links on "Current release information" for "Server Check" reports point to 1.9 docs.
          http://docs.moodle.org/19/en/admin/environment/moodle
          2. After the upgrade is complete "2.0.3+ (Build: 20110623)" points to http://docs.moodle.org/en/Release and "many other contributors." points to http://docs.moodle.org/en/Credits which is hard-coded (Already a BUG MDL-28135, for hard coded strings)
          3. Upgrading from 2.0 to 2.1 show 2.0 links. ("More help" on first page of upgrade points to http://docs.moodle.org/20/en/admin/versions), and Server check points to 2.0 links
          4. Moodle image Link location while installing 2.0 points to "http://docs.moodle.org/en/Administrator_documentation"

          Note :
          1, 2, and 3 "Fails" are only for upgrade. A Fresh install of 2.0 and 2.1 points to correct docs.

          Show
          Rajesh Taneja added a comment - Pass: 1. All links point fine in 1.9. 2. All links are fine in 2.0 except upgrade links. 3. All links are fine in 2.1 except upgrade links. Fails: 1. while upgrading from 1.9 to 2.0, links on "Current release information" for "Server Check" reports point to 1.9 docs. http://docs.moodle.org/19/en/admin/environment/moodle 2. After the upgrade is complete "2.0.3+ (Build: 20110623)" points to http://docs.moodle.org/en/Release and "many other contributors." points to http://docs.moodle.org/en/Credits which is hard-coded (Already a BUG MDL-28135 , for hard coded strings) 3. Upgrading from 2.0 to 2.1 show 2.0 links. ("More help" on first page of upgrade points to http://docs.moodle.org/20/en/admin/versions ), and Server check points to 2.0 links 4. Moodle image Link location while installing 2.0 points to "http://docs.moodle.org/en/Administrator_documentation" Note : 1, 2, and 3 "Fails" are only for upgrade. A Fresh install of 2.0 and 2.1 points to correct docs.
          Hide
          Aparup Banerjee added a comment - - edited

          yup thanks rajesh, 1,2,4 are hardcoded and need to be fixed (easy). all are links shown to admin only - please open up a quick mdl for this.
          3) is a redirection going on - remind Jordan to time that with release of 2.1

          Show
          Aparup Banerjee added a comment - - edited yup thanks rajesh, 1,2,4 are hardcoded and need to be fixed (easy). all are links shown to admin only - please open up a quick mdl for this. 3) is a redirection going on - remind Jordan to time that with release of 2.1
          Hide
          Eloy Lafuente (stronk7) added a comment -

          After chat with SamH, proposing to move forward to MDL-28134 and MDL-28135, where all the problems detected here will be decided and fixed.

          Awaiting for confirmation. Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - After chat with SamH, proposing to move forward to MDL-28134 and MDL-28135 , where all the problems detected here will be decided and fixed. Awaiting for confirmation. Ciao
          Hide
          Rajesh Taneja added a comment -

          Hello Eloy and Aparup,

          Updated MDL-29135, which will take care of fails 1, 2 and 4

          Cheers
          Rajesh

          Show
          Rajesh Taneja added a comment - Hello Eloy and Aparup, Updated MDL-29135 , which will take care of fails 1, 2 and 4 Cheers Rajesh
          Hide
          Martin Dougiamas added a comment -

          I commented on MDL-28134 and MDL-28135

          Show
          Martin Dougiamas added a comment - I commented on MDL-28134 and MDL-28135
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Sorry, guys so it's ok to pass this and let the new ones to handle needed changes? I think so, but need final +1 to consider this passed.

          Show
          Eloy Lafuente (stronk7) added a comment - Sorry, guys so it's ok to pass this and let the new ones to handle needed changes? I think so, but need final +1 to consider this passed.
          Hide
          Martin Dougiamas added a comment -

          Yes, I think that is correct. Those two new ones MDL-28134 and MDL-28135 have things to work on post-2.1. My +1 to pass this one.

          Show
          Martin Dougiamas added a comment - Yes, I think that is correct. Those two new ones MDL-28134 and MDL-28135 have things to work on post-2.1. My +1 to pass this one.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Passing, 2 followups raised to Blocker! Thanks everybody!

          Show
          Eloy Lafuente (stronk7) added a comment - Passing, 2 followups raised to Blocker! Thanks everybody!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Yay, this is now part of the just released Moodle 2.1 ! Thanks for all the hard work!

          Show
          Eloy Lafuente (stronk7) added a comment - Yay, this is now part of the just released Moodle 2.1 ! Thanks for all the hard work!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: