Uploaded image for project: '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
    • Status: Closed
    • Priority: 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:

      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."

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Hide
              nebgor Aparup Banerjee added a comment -

              SamH, whats your take on this fix?

              Show
              nebgor Aparup Banerjee added a comment - SamH, whats your take on this fix?
              Hide
              samhemelryk 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
              samhemelryk 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
              nebgor Aparup Banerjee added a comment -

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

              Show
              nebgor Aparup Banerjee added a comment - Hi Sam, i went with splitting that bit up. i've updated the patch.
              Hide
              samhemelryk 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
              samhemelryk 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
              nebgor 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
              nebgor 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
              samhemelryk 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
              samhemelryk 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
              nebgor 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
              nebgor 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
              nebgor Aparup Banerjee added a comment -

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

              Show
              nebgor Aparup Banerjee added a comment - created patches for 2.x branches too for print_error() linking to new moodle docs.
              Hide
              stronk7 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
              stronk7 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
              nebgor 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
              nebgor 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
              nebgor Aparup Banerjee added a comment -

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

              Show
              nebgor Aparup Banerjee added a comment - okay i've moved the function into setuplib.php , its up for integration review now.
              Hide
              stronk7 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
              stronk7 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
              stronk7 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
              stronk7 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
              nebgor 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
              nebgor 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
              nebgor Aparup Banerjee added a comment -

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

              Show
              nebgor Aparup Banerjee added a comment - i've updated my repo for 19 with (1) & (2)
              Hide
              stronk7 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
              stronk7 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
              stronk7 Eloy Lafuente (stronk7) added a comment -

              Integrated, thanks!

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - Integrated, thanks!
              Hide
              nebgor 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
              nebgor 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
              rajeshtaneja 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
              rajeshtaneja 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
              nebgor 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
              nebgor 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
              stronk7 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
              stronk7 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
              rajeshtaneja 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
              rajeshtaneja 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
              dougiamas Martin Dougiamas added a comment -

              I commented on MDL-28134 and MDL-28135

              Show
              dougiamas Martin Dougiamas added a comment - I commented on MDL-28134 and MDL-28135
              Hide
              stronk7 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
              stronk7 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
              dougiamas 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
              dougiamas 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
              stronk7 Eloy Lafuente (stronk7) added a comment -

              Passing, 2 followups raised to Blocker! Thanks everybody!

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - Passing, 2 followups raised to Blocker! Thanks everybody!
              Hide
              stronk7 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
              stronk7 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:
                    Fix Release Date:
                    1/Jul/11