DDC-1598: ProxyFactory makes assumptions on identifier getter code #2235

Closed
doctrinebot opened this Issue Jan 13, 2012 · 7 comments

2 participants

@doctrinebot

Jira issue originally created by user @ocramius:

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)\sfunction\sgetFieldname\s(\s)\s+{\s\$this\s->Fieldname\s;\s}

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

@doctrinebot

Comment created by @beberlei:

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? :)

@doctrinebot

Comment created by @ocramius:

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::OFFSETREQUIRED_BYWEBSERVICE;

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...

@doctrinebot

Comment created by @ocramius:

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.

@doctrinebot

Comment created by @beberlei:

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.

@doctrinebot

Issue was closed with resolution "Invalid"

@doctrinebot

Comment created by @ocramius:

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...

@doctrinebot

Comment created by @ocramius:

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
@beberlei beberlei was assigned by doctrinebot Dec 6, 2015
@doctrinebot doctrinebot added this to the 2.x milestone Dec 6, 2015
@doctrinebot doctrinebot closed this Dec 6, 2015
@doctrinebot doctrinebot added the Bug label Dec 7, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment