Moodle
  1. Moodle
  2. MDL-26433

contants defined in messaging need to be made more specific to avoid conflicts

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.0.1
    • Fix Version/s: 2.0.3
    • Component/s: Messages
    • Labels:
    • Database:
      Any
    • Affected Branches:
      MOODLE_20_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE
    • Rank:
      16137

      Description

      There are a bunch of defines using really common names like CONTACT_ID, VIEW_PARAM, IS_BLOCKER etc that can conflict because they are really generic. They must be more "unique" or be transformed into another beast.

      Eloy has also said that there use in optional_param() and building URLs is bad although Im not sure why. Hopefully he can provide more information on that.

        Activity

        Hide
        Andrew Davis added a comment -

        Still working on this but heres where the code lives

        repo: git://github.com/andyjdavis/moodle.git
        branch: MDL-26433_message_constants
        diff: https://github.com/andyjdavis/moodle/compare/master...MDL-26433_message_constants

        Show
        Andrew Davis added a comment - Still working on this but heres where the code lives repo: git://github.com/andyjdavis/moodle.git branch: MDL-26433 _message_constants diff: https://github.com/andyjdavis/moodle/compare/master...MDL-26433_message_constants
        Hide
        Eloy Lafuente (stronk7) added a comment - - edited

        Well,

        I really don't know how to explain it. The fist thing was the name itself that was too much "common" and could conflict with imported libraries or whatever. It seems that you already have fixed that.

        And the second objection are the uses that you are doing of those constants, or what constants are supposed to do. IMO constants are useful to define values that are related with the normal operations of the code, in other words, well-know-values, I mean, for example, it has perfect sense to create (note it is one invented case):

        define('MESSAGE_SENT', 1);
        define('MESSAGE_QUEUED', 0);
        

        And later use those constants in your code, both in conditions and sql params, and also when creating urls. It improves readability and causes, for sure less mistakes, so it's a good use of constants. There are a bunch of them being ok in you messaging code, like MESSAGE_USER_IS_CONTACT, MESSAGE_SHOW_ACTION_LINKS_IN_CONTACT_LIST... and surely more.

        But those constants must nor be used for anything not being values related with the code, like request param names or SQL param keys. For example, this is correct:

        $usergroup = optional_param('usergroup', MESSAGE_VIEW_UNREAD_MESSAGES, PARAM_ALPHANUMEXT);
        

        (you are getting the value of the 'usergroup' param and applying one well-known value (MESSAGE_VIEW_UNREAD_MESSAGES) if not present)

        But this is 100% wrong:

        $viewing = optional_param(MESSAGE_VIEW_PARAM, $usergroup, PARAM_ALPHANUMEXT);
        

        (getting the value of one param given by a constant. In this case, the constant is not one well-know value, but one name - one key)

        Exactly the same applies to SQL named params, you can use constant as values, but not as keys. And also to creation of links, you can use constants as values but not as keys.

        So, summarizing, any constant that you are using in the code as key is a candidate to be out. Constants are only values.

        Not sure if I've explained the difference properly, I hope so. Ciao

        Show
        Eloy Lafuente (stronk7) added a comment - - edited Well, I really don't know how to explain it. The fist thing was the name itself that was too much "common" and could conflict with imported libraries or whatever. It seems that you already have fixed that. And the second objection are the uses that you are doing of those constants, or what constants are supposed to do. IMO constants are useful to define values that are related with the normal operations of the code, in other words, well-know-values, I mean, for example, it has perfect sense to create (note it is one invented case): define('MESSAGE_SENT', 1); define('MESSAGE_QUEUED', 0); And later use those constants in your code, both in conditions and sql params, and also when creating urls. It improves readability and causes, for sure less mistakes, so it's a good use of constants. There are a bunch of them being ok in you messaging code, like MESSAGE_USER_IS_CONTACT, MESSAGE_SHOW_ACTION_LINKS_IN_CONTACT_LIST... and surely more. But those constants must nor be used for anything not being values related with the code, like request param names or SQL param keys. For example, this is correct: $usergroup = optional_param('usergroup', MESSAGE_VIEW_UNREAD_MESSAGES, PARAM_ALPHANUMEXT); (you are getting the value of the 'usergroup' param and applying one well-known value (MESSAGE_VIEW_UNREAD_MESSAGES) if not present) But this is 100% wrong: $viewing = optional_param(MESSAGE_VIEW_PARAM, $usergroup, PARAM_ALPHANUMEXT); (getting the value of one param given by a constant. In this case, the constant is not one well-know value, but one name - one key) Exactly the same applies to SQL named params, you can use constant as values, but not as keys. And also to creation of links, you can use constants as values but not as keys. So, summarizing, any constant that you are using in the code as key is a candidate to be out. Constants are only values . Not sure if I've explained the difference properly, I hope so. Ciao
        Hide
        Sam Marshall added a comment -

        To add to Eloy's comment I think it's a code style thing. In some other projects, for example, it would be expected that HTTP request parameter names like 'message' say would always be constants (so that the same parameter can be used in multiple places). Maybe also the same true for SQL table names. (In Java for instance, this is fairly common - but note that Java has really clear scoping of constants/enums so they won't 'escape' and affect other areas of the code or prevent other people from using the same name.)

        But in Moodle the code style is not to use constants for these things (parameters, SQL table or field names), only for values.

        Show
        Sam Marshall added a comment - To add to Eloy's comment I think it's a code style thing. In some other projects, for example, it would be expected that HTTP request parameter names like 'message' say would always be constants (so that the same parameter can be used in multiple places). Maybe also the same true for SQL table names. (In Java for instance, this is fairly common - but note that Java has really clear scoping of constants/enums so they won't 'escape' and affect other areas of the code or prevent other people from using the same name.) But in Moodle the code style is not to use constants for these things (parameters, SQL table or field names), only for values.
        Hide
        Andrew Davis added a comment - - edited

        Ok. I believe I've fixed up the use of constants. I personally much prefer to use constants pretty much everywhere but for the sake of consistency with the rest of Moodle I have made the changes.

        PULL-392

        repeating repo details for the convenience of one and all.
        repo: git://github.com/andyjdavis/moodle.git
        branch: MDL-26433_message_constants
        diff: https://github.com/andyjdavis/moodle/compare/master...MDL-26433_message_constants

        Show
        Andrew Davis added a comment - - edited Ok. I believe I've fixed up the use of constants. I personally much prefer to use constants pretty much everywhere but for the sake of consistency with the rest of Moodle I have made the changes. PULL-392 repeating repo details for the convenience of one and all. repo: git://github.com/andyjdavis/moodle.git branch: MDL-26433 _message_constants diff: https://github.com/andyjdavis/moodle/compare/master...MDL-26433_message_constants
        Hide
        Helen Foster added a comment -

        Andrew, thanks for this improvement.

        Show
        Helen Foster added a comment - Andrew, thanks for this improvement.

          People

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

            Dates

            • Created:
              Updated:
              Resolved: