Doctrine 2 - ORM
  1. Doctrine 2 - ORM
  2. DDC-1598

ProxyFactory makes assumptions on identifier getter code

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Critical Critical
    • Resolution: Invalid
    • Affects Version/s: 2.0, 2.0.1, 2.0.2, 2.0.3, 2.0.4, 2.0.5, 2.0.6, 2.0.7, 2.1, 2.1.1, 2.1.2, 2.1.3, 2.1.4, 2.1.5, 2.2-BETA1, 2.2-BETA2, Git Master
    • Fix Version/s: 2.2, 2.3, 2.x
    • Component/s: ORM
    • Security Level: All
    • Labels:
      None

      Description

      As of
      https://github.com/doctrine/doctrine2/blob/master/lib/Doctrine/ORM/Proxy/ProxyFactory.php#L214
      and
      https://github.com/doctrine/doctrine2/blob/master/lib/Doctrine/ORM/Proxy/ProxyFactory.php#L237
      the current ProxyFactory isn't actually checking if the identifier getter has logic in it.
      Current checks aren't enough/valid.

      In my opinion the check should be matching following:

      (public|protected)\s+function\s+getFieldname\s*(\s*)\s+

      {\s*\$this\s*->Fieldname\s*;\s*}

      Not really experienced with regex, but currently cannot come up with a more secure check.

        Activity

        Hide
        Marco Pivetta added a comment -

        As of IRC there's 3 ways (for now) to get this solved:

        • Some code scanner/stronger checks (difficult/problems with private methods/slow)
        • Regex (as of description)
        • Adding configuration (per-entity or per-method. Probably too messy)
        • Documenting it as "magic" of proxies and let the users be aware of it
        Show
        Marco Pivetta added a comment - As of IRC there's 3 ways (for now) to get this solved: Some code scanner/stronger checks (difficult/problems with private methods/slow) Regex (as of description) Adding configuration (per-entity or per-method. Probably too messy) Documenting it as "magic" of proxies and let the users be aware of it
        Hide
        Marco Pivetta added a comment -

        Nope, this code actually works only if the ID is set
        I'm not talking about changing IDs, it's just that getters and setters don't always reflect the class attributes...

        Show
        Marco Pivetta added a comment - Nope, this code actually works only if the ID is set I'm not talking about changing IDs, it's just that getters and setters don't always reflect the class attributes...
        Hide
        Benjamin Eberlei added a comment -

        This argument is void i just seen

        A method:

        public function getIdentifierField1()
        {
           return $this->identifierField1? $this->_myInitializationStuff() : null;
        } 
        

        Will only used when the id is not set anyways.

        In any other case Ids are Immutable and changing them in your code would break a lot more than just this smart proxy method generation.

        Show
        Benjamin Eberlei added a comment - This argument is void i just seen A method: public function getIdentifierField1() { return $ this ->identifierField1? $ this ->_myInitializationStuff() : null ; } Will only used when the id is not set anyways. In any other case Ids are Immutable and changing them in your code would break a lot more than just this smart proxy method generation.
        Hide
        Marco Pivetta added a comment -

        Also don't know what stuff like EAccelerator (known in this Jira as of it's fantastic idea about stripping comments) would make the check of the 4 lines like.

        Show
        Marco Pivetta added a comment - Also don't know what stuff like EAccelerator (known in this Jira as of it's fantastic idea about stripping comments) would make the check of the 4 lines like.
        Hide
        Marco Pivetta added a comment -

        First of all it is a question of concept. Doctrine shouldn't assume anything about entities outside of their fields. It already introduces a level of complication when we explain how to clone/serialize objects, which was very confusing.

        Asking for the purpose of an identifier field getter method is in my opinion again an attempt of making assumptions over user's code...

        What if the user wrote something like:

        public function getIdentifierField1()

        { return $this->identifierField1? $this->_myInitializationStuff() : null; }

        private function _myInitializationStuff()

        { //open files, check stuff, make things difficult for the D2 team :D }

        For instance, opening file handlers, sockets, whatever... This is a case that would break the entity because of optimization introduced by the ProxyFactory. (not to mention when getIdentifierField1 does some conversion, like return $this->identifierField1 + self::OFFSET_REQUIRED_BY_WEBSERVICE;

        I'm not arguing about the validity of this optimization, but that the checks are too lazy.

        I've read something about moving the ProxyFactory to common and using code scanner tools, and the check should be about applying the optimization only when the form is

        return $this->identifierField1;

        That's why I put the example of the regex. That would probably not be as safe as using some reflection-based tool, but surely more than just checking if the code is <= 4 lines...

        Show
        Marco Pivetta added a comment - First of all it is a question of concept. Doctrine shouldn't assume anything about entities outside of their fields. It already introduces a level of complication when we explain how to clone/serialize objects, which was very confusing. Asking for the purpose of an identifier field getter method is in my opinion again an attempt of making assumptions over user's code... What if the user wrote something like: public function getIdentifierField1() { return $this->identifierField1? $this->_myInitializationStuff() : null; } private function _myInitializationStuff() { //open files, check stuff, make things difficult for the D2 team :D } For instance, opening file handlers, sockets, whatever... This is a case that would break the entity because of optimization introduced by the ProxyFactory. (not to mention when getIdentifierField1 does some conversion, like return $this->identifierField1 + self::OFFSET_REQUIRED_BY_WEBSERVICE; I'm not arguing about the validity of this optimization, but that the checks are too lazy. I've read something about moving the ProxyFactory to common and using code scanner tools, and the check should be about applying the optimization only when the form is return $this->identifierField1; That's why I put the example of the regex. That would probably not be as safe as using some reflection-based tool, but surely more than just checking if the code is <= 4 lines...
        Hide
        Benjamin Eberlei added a comment -

        Can you explain why you think this is necessary?

        You are right an id method with logic could exist in 4 lines of code, but for what purpose?

        Show
        Benjamin Eberlei added a comment - Can you explain why you think this is necessary? You are right an id method with logic could exist in 4 lines of code, but for what purpose?

          People

          • Assignee:
            Benjamin Eberlei
            Reporter:
            Marco Pivetta
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: