mirror of
https://github.com/doctrine/orm.git
synced 2026-03-23 22:42:18 +01:00
DDC-1598: ProxyFactory makes assumptions on identifier getter code #2006
Reference in New Issue
Block a user
Delete Branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Originally created by @doctrinebot on GitHub (Jan 13, 2012).
Originally assigned to: @beberlei on GitHub.
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 commented on GitHub (Jan 15, 2012):
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 commented on GitHub (Jan 15, 2012):
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::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...
@doctrinebot commented on GitHub (Jan 15, 2012):
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 commented on GitHub (Jan 16, 2012):
Comment created by @beberlei:
This argument is void i just seen
A method:
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 commented on GitHub (Jan 16, 2012):
Issue was closed with resolution "Invalid"
@doctrinebot commented on GitHub (Jan 16, 2012):
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 commented on GitHub (Jan 16, 2012):
Comment created by @ocramius:
As of IRC there's 3 ways (for now) to get this solved: