Moodle
  1. Moodle
  2. MDL-30977

page API, check and update DocBlock

    Details

    • Rank:
      37381

      Description

      Check and update documentation, so that it should comply with moodle coding guidelines.
      Following needs to be updated/checked for page api

      1. DocBlock for page and functions.
      2. All the files should be checked/updated.

      Note: You can create sub-tasks, so as to avoid bulk integration.

        Issue Links

          Activity

          Hide
          Sam Hemelryk added a comment -

          Hi guys,

          I've completed the phpdocs review for the Page API (moodle_page).

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi guys, I've completed the phpdocs review for the Page API (moodle_page). Cheers Sam
          Hide
          Sam Hemelryk added a comment -

          Peer-reviewer + integrator: There are lots of deprecated classes and functions in lib/pagelib.php, I've updated the PHPdocs for them as part of my changes but please note I have created MDL-31023 to remove them before the next release.
          This issue blocks it of course as the changes are in the same area.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Peer-reviewer + integrator: There are lots of deprecated classes and functions in lib/pagelib.php, I've updated the PHPdocs for them as part of my changes but please note I have created MDL-31023 to remove them before the next release. This issue blocks it of course as the changes are in the same area. Cheers Sam
          Hide
          Sam Hemelryk added a comment -

          Forgot to add all changes are phpdoc or whitespace fixes and it cleanly cherry-picks from master to 22.

          Show
          Sam Hemelryk added a comment - Forgot to add all changes are phpdoc or whitespace fixes and it cleanly cherry-picks from master to 22.
          Hide
          Rajesh Taneja added a comment -

          Thanks for working on this, there are few things you might want to consider:

          1. subpackage is not used anymore, and package is full frankenstyle. @package core_page
          2. Add category tag to classes/functions
          3. @var dockblock (was updated after david and others accepted it to be one-liner)

          Rest looks Great

          Show
          Rajesh Taneja added a comment - Thanks for working on this, there are few things you might want to consider: subpackage is not used anymore, and package is full frankenstyle. @package core_page Add category tag to classes/functions @var dockblock (was updated after david and others accepted it to be one-liner) Rest looks Great
          Hide
          Michael de Raadt added a comment -

          Hi, Sam.

          I think you were first to put something up for peer review, so I thought I would have a go at peer-reviewing yours first.

          Some comments:

          • For the page class, at line 70 to 90 there are some comments that include the term "return". As these comments are describing member variables, perhaps the word return is not appropriate. Perhaps "is" could be used instead, or the verb could be dropped.
          • In the pagelayout description at line 181, there should be a comma: "the theme in use, or one of its parents." Also, the final sentence in this comment seems like it needs some more information, like "so you should...".
          • In the subpage description at line 199 there is an incomplete sentence "This is useful for pages"; is there a because?
          • It seems to me that blockseditingcap should be a constant rather than a member variable, although that's beyond the scope of this exercise.

          I'm up to about line 400 and saving this comment so far.

          Show
          Michael de Raadt added a comment - Hi, Sam. I think you were first to put something up for peer review, so I thought I would have a go at peer-reviewing yours first. Some comments: For the page class, at line 70 to 90 there are some comments that include the term "return". As these comments are describing member variables, perhaps the word return is not appropriate. Perhaps "is" could be used instead, or the verb could be dropped. In the pagelayout description at line 181, there should be a comma: "the theme in use, or one of its parents." Also, the final sentence in this comment seems like it needs some more information, like "so you should...". In the subpage description at line 199 there is an incomplete sentence "This is useful for pages"; is there a because? It seems to me that blockseditingcap should be a constant rather than a member variable, although that's beyond the scope of this exercise. I'm up to about line 400 and saving this comment so far.
          Hide
          Sam Hemelryk added a comment -

          Thanks for the feedback guys.

          Raj:
          1,2/ indeed @subpackage => @category came and @package definition changed since I'd done this work. I've updated them now to reflect the changes to the coding style.
          3/ I've updated the docs now, they aren't one line because that doesn't work for most of the parameters within pagelib as they have multi line comments, however I've moved the @var to the start of the first line which seems to be what they are going for. Personally I think a mixture of single line and multiple line is a requirement, perhaps we just need to require that @var comes at the start... although by phpdoc standard either is perfectly acceptable (lol I think we are well passed the phpdoc standard now).

          Michael:
          1/ I've updated the comments there, certainly return is more in context to the magic methods processing the requests for this vars. I've updated it now removing `return` in all cases and altering definitions to make it read more like a standard property.
          2/ Added the comma
          3/ Removed that line it shouldn't have been there.
          4/ The blockseditingcap is overridden by several pages. Pretty sure under the current structure of our block code (which is horribly antiquated) we still need it to be flexible.
          5/ Just as a note Michael line numbers will have changed now sorry.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Thanks for the feedback guys. Raj: 1,2/ indeed @subpackage => @category came and @package definition changed since I'd done this work. I've updated them now to reflect the changes to the coding style. 3/ I've updated the docs now, they aren't one line because that doesn't work for most of the parameters within pagelib as they have multi line comments, however I've moved the @var to the start of the first line which seems to be what they are going for. Personally I think a mixture of single line and multiple line is a requirement, perhaps we just need to require that @var comes at the start... although by phpdoc standard either is perfectly acceptable (lol I think we are well passed the phpdoc standard now). Michael: 1/ I've updated the comments there, certainly return is more in context to the magic methods processing the requests for this vars. I've updated it now removing `return` in all cases and altering definitions to make it read more like a standard property. 2/ Added the comma 3/ Removed that line it shouldn't have been there. 4/ The blockseditingcap is overridden by several pages. Pretty sure under the current structure of our block code (which is horribly antiquated) we still need it to be flexible. 5/ Just as a note Michael line numbers will have changed now sorry. Cheers Sam
          Hide
          Michael de Raadt added a comment -

          OK. Back to it.

          Some more comments:

          • The type bool should be used instead of boolean, a quick search and replace
          • In the magic set function short description, line 737, perhaps "prevents" should be "avoids"
          • In the description of set_activity_record(), line 961, perhaps instead of "cm" you could say "module (cm)"
          • It would be good if the description of set_pagelayout() parameter $pagelayout included links to where possible values could be found in docs
          • The description for ensure_category_loaded(), line 1659, should be "This function ensures that the category of the current course has been loaded, and if not, the function loads it now."
          • The description for ensure_categories_loaded(), line 1700, should be "Ensures that the category the current course is within, as well as all of its parent categories, have been loaded." Also, the return for this function is probably unnecessary.
          • The description for set_legacy_page_object(), line 1892, says it returns the legacy page object; is that true?

          Very meticulously done, Sam. I hope other docs updates are as good.

          Show
          Michael de Raadt added a comment - OK. Back to it. Some more comments: The type bool should be used instead of boolean, a quick search and replace In the magic set function short description, line 737, perhaps "prevents" should be "avoids" In the description of set_activity_record(), line 961, perhaps instead of "cm" you could say "module (cm)" It would be good if the description of set_pagelayout() parameter $pagelayout included links to where possible values could be found in docs The description for ensure_category_loaded(), line 1659, should be "This function ensures that the category of the current course has been loaded, and if not, the function loads it now." The description for ensure_categories_loaded(), line 1700, should be "Ensures that the category the current course is within, as well as all of its parent categories, have been loaded." Also, the return for this function is probably unnecessary. The description for set_legacy_page_object(), line 1892, says it returns the legacy page object; is that true? Very meticulously done, Sam. I hope other docs updates are as good.
          Hide
          Sam Hemelryk added a comment -

          Hi Michael,

          Thanks for getting to the end of it.
          I've made the requested changes in most cases and have pushed up the revised branches again now.
          In reply to your points:

          1. done
          2. I've reworded the comment on the __set method to make it clearer that it is there to help developers.
          3. done
          4. I've updated the comment for this, there is one reference to layouts in the docs from a very early theme document I wrote. I've linked to that and mentioned that the only place to find a truly up to date list of layouts for your version of Moodle is in theme/base/config.php.
          5. done
          6. done
          7. Yip it is correct, I've updated the docs adding an @return now.

          All your's again Michael, if you're happy with it put it up for integration. Otherwise list out the changes

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Michael, Thanks for getting to the end of it. I've made the requested changes in most cases and have pushed up the revised branches again now. In reply to your points: done I've reworded the comment on the __set method to make it clearer that it is there to help developers. done I've updated the comment for this, there is one reference to layouts in the docs from a very early theme document I wrote. I've linked to that and mentioned that the only place to find a truly up to date list of layouts for your version of Moodle is in theme/base/config.php. done done Yip it is correct, I've updated the docs adding an @return now. All your's again Michael, if you're happy with it put it up for integration. Otherwise list out the changes Cheers Sam
          Hide
          Michael de Raadt added a comment -

          I'm happy with the current changes, but before pushing this to integration, I thought I should ask if there are functions related to page around CORE code that may need to be checked. I can't think of any.

          Show
          Michael de Raadt added a comment - I'm happy with the current changes, but before pushing this to integration, I thought I should ask if there are functions related to page around CORE code that may need to be checked. I can't think of any.
          Hide
          Martin Dougiamas added a comment -

          Can you please extract a list of the names of all the the classes and functions you've tagged with "@category page" (ie the actual API that people out in pluginland will use) on http://docs.moodle.org/dev/Page_API and then try to massage them into an overview of what page is and how to use it, what order to call things, what is required/optional and so on?

          Perhaps a big documented example would be best.

          Show
          Martin Dougiamas added a comment - Can you please extract a list of the names of all the the classes and functions you've tagged with "@category page" (ie the actual API that people out in pluginland will use) on http://docs.moodle.org/dev/Page_API and then try to massage them into an overview of what page is and how to use it, what order to call things, what is required/optional and so on? Perhaps a big documented example would be best.
          Hide
          Sam Hemelryk added a comment -

          Michael: Thanks, indeed there arn't any callbacks or extensions in core code that require additional documentation.
          Putting this up for integration now.

          Martin: No probs will start work on a descriptive document shortly. Do you know is there a document I can reference for structure and target audience?

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Michael: Thanks, indeed there arn't any callbacks or extensions in core code that require additional documentation. Putting this up for integration now. Martin: No probs will start work on a descriptive document shortly. Do you know is there a document I can reference for structure and target audience? Cheers Sam
          Hide
          Eloy Lafuente (stronk7) added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Sam Hemelryk added a comment -

          http://docs.moodle.org/dev/Page_API has been written now (and everything has been rebased)

          Show
          Sam Hemelryk added a comment - http://docs.moodle.org/dev/Page_API has been written now (and everything has been rebased)
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Moving all the phpdocs-related issues out from current integration, will be examined next week.

          Note that all these issues are going to be applied exclusively to master, that has been summarily decided by the integration team after hea(t)ring everybody.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Moving all the phpdocs-related issues out from current integration, will be examined next week. Note that all these issues are going to be applied exclusively to master, that has been summarily decided by the integration team after hea(t)ring everybody. Ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Sam Hemelryk added a comment -

          Rebase (and removed 22 branch)

          Show
          Sam Hemelryk added a comment - Rebase (and removed 22 branch)
          Hide
          Eloy Lafuente (stronk7) added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -
          • package core_page is incorrect.
          • lib/pagelib.php:
            • get_id() #2120 (missing docs)
            • moodle_page::__set() #743 (incomplete param list)
            • moodle_page::set_state() #835 (incomplete param list)
            • moodle_page::set_course() #860 (incomplete param list)
            • moodle_page::print_header() #1914 (incomplete param list)
            • moodle_page::set_popup_notification_allowed() #1994 (incomplete param list)

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - package core_page is incorrect. lib/pagelib.php: get_id() #2120 (missing docs) moodle_page::__set() #743 (incomplete param list) moodle_page::set_state() #835 (incomplete param list) moodle_page::set_course() #860 (incomplete param list) moodle_page::print_header() #1914 (incomplete param list) moodle_page::set_popup_notification_allowed() #1994 (incomplete param list) Ciao
          Hide
          Sam Hemelryk added a comment -

          Thanks Eloy, have updated things now and all ready to go again.
          A couple of things I didn't change because they are code and not docs.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Thanks Eloy, have updated things now and all ready to go again. A couple of things I didn't change because they are code and not docs. Cheers Sam
          Hide
          Sam Hemelryk added a comment -

          Updated once more sorry Eloy

          Show
          Sam Hemelryk added a comment - Updated once more sorry Eloy
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integrated, thanks!

          Note: I've added one small commit fixing 2 little problems.

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated, thanks! Note: I've added one small commit fixing 2 little problems.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Nobody has tested this, passed.

          Show
          Eloy Lafuente (stronk7) added a comment - Nobody has tested this, passed.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Some changes to Moodle should be milestones in the project by themselves.

          This is not the case and your fix is not so important, but your collaboration is highly appreciated, thanks!

          Closing as fixed, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Some changes to Moodle should be milestones in the project by themselves. This is not the case and your fix is not so important, but your collaboration is highly appreciated, thanks! Closing as fixed, ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: