Doctrine 2 - ORM
  1. Doctrine 2 - ORM
  2. DDC-536

Remove the _ prefix from private and protected members

    Details

    • Type: Improvement Improvement
    • Status: Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: None
    • Fix Version/s: 3.0
    • Component/s: None
    • Security Level: All
    • Labels:
      None

      Description

      The reasoning is simple: The prefix "_" is usually either used for easier distinction of instance variables from other, i.e. local variables, instead of always using "this." (often seen in C#), or it is used to signal that a member is not meant to be accessed from outside of the class when the language does not have visibility modifiers (PHP4).

      Since you always have to use "$this->" in PHP5+ when accessing instance members and there are visibility modifiers, the "_" is largely superfluous and just makes the verbose OO code even more verbose.

      Maybe the following find/replace steps will do the job almost completely:

      "private $_" => "private $"
      "protected $_" => "protected $"
      "$this->_" => "$this->"
      

        Activity

        Roman S. Borschel created issue -
        Hide
        Benjamin Eberlei added a comment -

        i just found a possible BC issue with this.

        EntityRepository is allowed to be extended by us, it has several variables that are underscore prefixed. How to proceed in this case?

        Show
        Benjamin Eberlei added a comment - i just found a possible BC issue with this. EntityRepository is allowed to be extended by us, it has several variables that are underscore prefixed. How to proceed in this case?
        Hide
        Roman S. Borschel added a comment -

        I know but its not really a problem I think. We should just decide whether we make them private in the first place and provide getters instead (which would have avoided this problem in the first place).

        Show
        Roman S. Borschel added a comment - I know but its not really a problem I think. We should just decide whether we make them private in the first place and provide getters instead (which would have avoided this problem in the first place).
        Hide
        Roman S. Borschel added a comment -

        Leaving the prefixes on the repository class only is also an option... but I dont think thats necessary.

        Show
        Roman S. Borschel added a comment - Leaving the prefixes on the repository class only is also an option... but I dont think thats necessary.
        Hide
        Benjamin Eberlei added a comment -

        can we commit getters for Beta 1 then? We could give everyone a period until Beta 2 to fix their code and then make the change.

        EntityRepository is the only class that is meant to be userland extendable to my knowledge, so this should be the only problem to adress

        Show
        Benjamin Eberlei added a comment - can we commit getters for Beta 1 then? We could give everyone a period until Beta 2 to fix their code and then make the change. EntityRepository is the only class that is meant to be userland extendable to my knowledge, so this should be the only problem to adress
        Hide
        Roman S. Borschel added a comment -

        Yes, you can add getters and commit right away if you want. Plus adding a note on the upgrade document that direct access of these properties is deprecated.

        Show
        Roman S. Borschel added a comment - Yes, you can add getters and commit right away if you want. Plus adding a note on the upgrade document that direct access of these properties is deprecated.
        Hide
        Roman S. Borschel added a comment -

        Persisters will be also extensible some day in userland but they need more polish for that, I've already started with it

        Show
        Roman S. Borschel added a comment - Persisters will be also extensible some day in userland but they need more polish for that, I've already started with it
        Hide
        Johnny Peck added a comment -

        Is this still planned? Searching the code base finds this is not being implemented. It would be a good idea to implement the change sooner than later if it will be done at all. Also, +1 for the change. It makes complete sense.

        Show
        Johnny Peck added a comment - Is this still planned? Searching the code base finds this is not being implemented. It would be a good idea to implement the change sooner than later if it will be done at all. Also, +1 for the change. It makes complete sense.
        Benjamin Eberlei made changes -
        Field Original Value New Value
        Workflow jira [ 11261 ] jira-feedback [ 13848 ]
        Benjamin Eberlei made changes -
        Workflow jira-feedback [ 13848 ] jira-feedback2 [ 15712 ]
        Benjamin Eberlei made changes -
        Workflow jira-feedback2 [ 15712 ] jira-feedback3 [ 17969 ]
        Guilherme Blanco made changes -
        Assignee Roman S. Borschel [ romanb ] Guilherme Blanco [ guilhermeblanco ]
        Fix Version/s 3.0 [ 10129 ]
        Fix Version/s 2.0 [ 10021 ]
        Hide
        Guilherme Blanco added a comment -

        Moving to 3.0 as this can potentially create BC breaks

        Show
        Guilherme Blanco added a comment - Moving to 3.0 as this can potentially create BC breaks
        Guilherme Blanco made changes -
        Issue Type Task [ 3 ] Improvement [ 4 ]

        This list may be incomplete, as errors occurred whilst retrieving source from linked applications:

        • Request to http://www.doctrine-project.org/fisheye/ failed: Error in remote call to 'FishEye 0 (http://www.doctrine-project.org/fisheye/)' (http://www.doctrine-project.org/fisheye) [AbstractRestCommand{path='/rest-service-fe/search-v1/crossRepositoryQuery', params={query=DDC-536, expand=changesets[0:20].revisions[0:29],reviews}, methodType=GET}] : Received status code 503 (Service Temporarily Unavailable)

          People

          • Assignee:
            Guilherme Blanco
            Reporter:
            Roman S. Borschel
          • Votes:
            1 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated: