Moodle
  1. Moodle
  2. MDL-31711

invalid usages of '@property' found for class variables that are not really 'magic' variables.

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3
    • Fix Version/s: 2.3
    • Component/s: phpdoc
    • Labels:

      Description

      i was working on MDL-25027 when i came across usages of @property - lib/externallib.php class external_value's phpdoc has :

          /** @property mixed $type value type PARAM_XX */
          public $type;

      see - http://manual.phpdoc.org/HTMLSmartyConverter/HandS/phpDocumentor/tutorial_tags.property.pkg.html

      looking it up i read this about @property - '....in the "Property Summary" listing of your class variables, but you will NOT see "$regular", "$foo", or "$bar" there...'
      clearly some usages of @property should actually be @var as we DO want them documented and shown in the class's property summary.

      @property is really only for magic variables where we want to HIDE them from normal users.

        Gliffy Diagrams

          Activity

          Hide
          Aparup Banerjee added a comment -

          I've put up a patch that will now have these variables show up in generated documentation properly.


          ps: i've ignored other @property usages where they are simply used as a supplement in a class's docblock as part of the doc block's description :

          • mod/lesson/locallib.php
          • calender/lib.php
            Those won't effectively hide any documentation as the variables are marked with @var still anyway.
          Show
          Aparup Banerjee added a comment - I've put up a patch that will now have these variables show up in generated documentation properly. ps: i've ignored other @property usages where they are simply used as a supplement in a class's docblock as part of the doc block's description : mod/lesson/locallib.php calender/lib.php Those won't effectively hide any documentation as the variables are marked with @var still anyway.
          Hide
          Ankit Agarwal added a comment -

          Hi Aparup,
          I also found some instances of @property in lib/externallib.php, which I don't think are "magic" variables either.
          Please have a look if these needs to be changed to @var or not.

          The patch looks great. +1 to integrate, once above mentioned instances are sorted.
          Thanks

          Show
          Ankit Agarwal added a comment - Hi Aparup, I also found some instances of @property in lib/externallib.php, which I don't think are "magic" variables either. Please have a look if these needs to be changed to @var or not. The patch looks great. +1 to integrate, once above mentioned instances are sorted. Thanks
          Hide
          Aparup Banerjee added a comment -

          ah some changes were done in MDL-30994 already now.

          Show
          Aparup Banerjee added a comment - ah some changes were done in MDL-30994 already now.
          Hide
          Aparup Banerjee added a comment -

          i've made changes to lib/externallib.php , please review and push up for integration

          Show
          Aparup Banerjee added a comment - i've made changes to lib/externallib.php , please review and push up for integration
          Hide
          Ankit Agarwal added a comment -

          Looks good, sending for integration.
          Thanks

          Show
          Ankit Agarwal added a comment - Looks good, sending for integration. Thanks
          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 -

          Thanks Apu, this has been integrated now

          Show
          Sam Hemelryk added a comment - Thanks Apu, this has been integrated now
          Hide
          Sam Hemelryk added a comment -

          Passes fine

          Show
          Sam Hemelryk added a comment - Passes fine
          Hide
          Aparup Banerjee added a comment -

          The code here has been spread to upstream moodle repositories and mirrors for anyone to use .

          Closing, have a good weekend!

          Show
          Aparup Banerjee added a comment - The code here has been spread to upstream moodle repositories and mirrors for anyone to use . Closing, have a good weekend!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: