Details

    • Rank:
      37378

      Description

      Check and update documentation, so that it should comply with moodle coding guidelines.
      Following needs to be updated/checked for forms 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
          Rajesh Taneja added a comment -

          Still working on it, adding Pull branches for reference.

          Show
          Rajesh Taneja added a comment - Still working on it, adding Pull branches for reference.
          Hide
          Rajesh Taneja added a comment -

          will update 2.2 after peer review.

          Show
          Rajesh Taneja added a comment - will update 2.2 after peer review.
          Hide
          Martin Dougiamas added a comment -

          This is the docs page: http://docs.moodle.org/dev/Form_API

          Show
          Martin Dougiamas added a comment - This is the docs page: http://docs.moodle.org/dev/Form_API
          Hide
          Rajesh Taneja added a comment -

          Can you please provide feedback, before I create 2.2 branch and push it for integration.

          Show
          Rajesh Taneja added a comment - Can you please provide feedback, before I create 2.2 branch and push it for integration.
          Hide
          Rajesh Taneja added a comment -

          Updated form API doc page

          Show
          Rajesh Taneja added a comment - Updated form API doc page
          Hide
          Sam Hemelryk added a comment - - edited

          Hi Raj,

          I've been looking at this just now, the following are my notes:

          1. The @abstract tag is only valid in PHP 4, PHP 5 has a keyword abstract. (pasted from the phpdoc manual) No need to specify that any more as we require php5 of course exception to that is if we have old code with abstract classes that are presumed abstract and not actually noted as abstract by the keyword.
          2. Several boolean and integer's around the place.
          3. formslib.php
            • moodleform class
              • $customdata doesn't have to be an array it can be anything
              • Please fix the alignment of multiline params in moodleform method
              • save_files method needs @deprecated tag
              • get_new_filename comma after $elname should be removed
              • save_stored_file missing an @param
            • MoodleQuickForm class
              • MoodleQuickForm method $action is either a string of a moodle_url
              • accept $renderer appears to be a HTML_QuickForm_Renderer object
              • setType includes a limited set of examples for PARAM_WHATEVER uses. Not your doing just noting that is totally in the wrong place. If we do provide an example it should be a method example not a param example. Actually PARAM_ACTION is deprecated and a bad example anyway lol.. would you mind tiding that up somehow Raj?
              • exportValues missing @return
          4. Several elements have a typo in the setHelpButton method: $help => $helpbuttonargs and also missin deprecated tags etc.
          5. Missing docblock for MoodleQuickForm_checkbox constructor
          6. checkbox also missing deprecated etc from setHelpButton
          7. Please tidy up alignment on MoodleQuickForm_date_selector line 44.. that is a whole new way of doing a docblock and we need to minimise the number of different ways we do things. (in a couple of other places as well wont be mentioned again)
          8. date_selector accept $renderer is a HTML_QuickForm_Renderer (same in other places won't be mentioned again)
          9. form_filemanaer_x missing phpdoc block for property and constructor
          10. MoodleQuickForm_htmleditor missing php doc blocks for properties and constructor
          11. selectwithlink::removeOptions incorrect @param

          On an interesting note I noticed several docblocks like the following:

          /** @var string String with the html for hidden params passed in as part of a moodle_url
           * object for the action. Output in the form. */
          

          Do you know whether that sort of docblock validates OK? (I've never seen it before these changes)
          Actually you introduced a couple of different variants to this.
          I think we need to really limit the number of different ways we create phpdoc blocks.
          I think the following two should be acceptable and the rest be changed:

          /** A single line comment **/
          
          /**
           * A multiline or single comment
           * should look like this
           */
          

          Again all about consistency and example. Perhaps I'm being to up tight about it but the more variations I see the less I like our documentation.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - - edited Hi Raj, I've been looking at this just now, the following are my notes: The @abstract tag is only valid in PHP 4, PHP 5 has a keyword abstract. (pasted from the phpdoc manual) No need to specify that any more as we require php5 of course exception to that is if we have old code with abstract classes that are presumed abstract and not actually noted as abstract by the keyword. Several boolean and integer's around the place. formslib.php moodleform class $customdata doesn't have to be an array it can be anything Please fix the alignment of multiline params in moodleform method save_files method needs @deprecated tag get_new_filename comma after $elname should be removed save_stored_file missing an @param MoodleQuickForm class MoodleQuickForm method $action is either a string of a moodle_url accept $renderer appears to be a HTML_QuickForm_Renderer object setType includes a limited set of examples for PARAM_WHATEVER uses. Not your doing just noting that is totally in the wrong place. If we do provide an example it should be a method example not a param example. Actually PARAM_ACTION is deprecated and a bad example anyway lol.. would you mind tiding that up somehow Raj? exportValues missing @return Several elements have a typo in the setHelpButton method: $help => $helpbuttonargs and also missin deprecated tags etc. Missing docblock for MoodleQuickForm_checkbox constructor checkbox also missing deprecated etc from setHelpButton Please tidy up alignment on MoodleQuickForm_date_selector line 44.. that is a whole new way of doing a docblock and we need to minimise the number of different ways we do things. (in a couple of other places as well wont be mentioned again) date_selector accept $renderer is a HTML_QuickForm_Renderer (same in other places won't be mentioned again) form_filemanaer_x missing phpdoc block for property and constructor MoodleQuickForm_htmleditor missing php doc blocks for properties and constructor selectwithlink::removeOptions incorrect @param On an interesting note I noticed several docblocks like the following: /** @ var string String with the html for hidden params passed in as part of a moodle_url * object for the action. Output in the form. */ Do you know whether that sort of docblock validates OK? (I've never seen it before these changes) Actually you introduced a couple of different variants to this. I think we need to really limit the number of different ways we create phpdoc blocks. I think the following two should be acceptable and the rest be changed: /** A single line comment **/ /** * A multiline or single comment * should look like this */ Again all about consistency and example. Perhaps I'm being to up tight about it but the more variations I see the less I like our documentation. Cheers Sam
          Hide
          Sam Hemelryk added a comment -

          BTW all in all very good dude, most of the noted things are just very minor and are easily missed.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - BTW all in all very good dude, most of the noted things are just very minor and are easily missed. Cheers Sam
          Hide
          Rajesh Taneja added a comment -

          Thanks Sam,
          Branch updated with all the changes (you suggested and few more.)
          As discussed, pushing it up for integration review.

          Show
          Rajesh Taneja added a comment - Thanks Sam, Branch updated with all the changes (you suggested and few more.) As discussed, pushing it up for integration review.
          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
          Rajesh Taneja added a comment -

          Branch re-based.

          Show
          Rajesh Taneja added a comment - Branch re-based.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          missing repository url

          Show
          Eloy Lafuente (stronk7) added a comment - missing repository url
          Hide
          Rajesh Taneja added a comment -

          Sorry Eloy
          Added repository URL.

          Show
          Rajesh Taneja added a comment - Sorry Eloy Added repository URL.
          Hide
          Sam Hemelryk added a comment -

          Thanks Raj, this has been integrated now

          Show
          Sam Hemelryk added a comment - Thanks Raj, this has been integrated now
          Hide
          Adrian Greeve added a comment -

          This all looks good. Thanks.

          Show
          Adrian Greeve added a comment - This all looks good. Thanks.
          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
          Hide
          Aparup Banerjee added a comment -

          added MDL-31294 which is removing deprecated setHelpButton()

          also handled there are :

          • removing MoodleQuickForm_file
          • removing MoodleQuickForm_format
          Show
          Aparup Banerjee added a comment - added MDL-31294 which is removing deprecated setHelpButton() also handled there are : removing MoodleQuickForm_file removing MoodleQuickForm_format

            People

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

              Dates

              • Created:
                Updated:
                Resolved: