[DCOM-272] Proxy generator doesn't understand splat operator (PHP 5.6 argument unpacking) Created: 09/Feb/15  Updated: 10/Feb/15

Status: Open
Project: Doctrine Common
Component/s: None
Affects Version/s: 2.4.1
Fix Version/s: None

Type: Bug Priority: Major
Reporter: Ruud Kamphuis Assignee: Marco Pivetta
Resolution: Unresolved Votes: 0
Labels: code, php-5.6, proxy, proxy-generator, splat

Issue Links:
Dependency
depends on DCOM-273 [GH-353] Support for Variadic argumen... Open
Reference
is referenced by DCOM-273 [GH-353] Support for Variadic argumen... Open

 Description   

I have a function inside my entity that uses variadic arguments (argument unpacking) using the PHP 5.6 splat operator:

public function addReference(...$references) {}

But the proxy generator doesn't take that into account when generating the proxies:

    /**
     * {@inheritDoc}
     */
    public function addReference($references)
    {

        $this->__initializer__ && $this->__initializer__->__invoke($this, 'addReference', array($references));

        return parent::addReference($references);
    }

Resulting in this error:

Runtime Notice: Declaration of Proxies\__CG__\AccountingBundle\Entity\Journal::addReference() should be compatible with AccountingBundle\Entity\Journal::addReference(...$references)

The solution would be to add this to the ProxyGenerator generateMethods method:

                if (method_exists($param, 'isVariadic')) {
                    if ($param->isVariadic()) {
                        $parameterString .= '...';
                        $argumentString .= '...';
                    }
                }


 Comments   
Comment by Marco Pivetta [ 09/Feb/15 ]

Ruud Kamphuis I knew this was coming, heh.

Can you actually provide the fix as a PR with a test?

Comment by Ruud Kamphuis [ 09/Feb/15 ]

haha I really want to create PR for it, but I'm just curious how we are going to test this on multiple PHP versions? Because isVariadic only works in PHP 5.6. Do the ProxyGenerator tests compare the output of the generated PHP scripts?

Comment by Marco Pivetta [ 09/Feb/15 ]

Ruud Kamphuis something like following:

Unable to find source-code formatter for language: php. Available languages are: actionscript, html, java, javascript, none, sql, xhtml, xml
public function testZehSplatThing()
{
    if ( ! method_exists('ReflectionParameter', 'isVariadic')) {
        $this->markTestSkipped('PHP 5.6+ only test');
    }

    // ...
}
Comment by Ruud Kamphuis [ 09/Feb/15 ]

I created a PR for this https://github.com/doctrine/common/pull/353

Could you check my question regarding the __invoke call?

Comment by Doctrine Bot [ 09/Feb/15 ]

A related Github Pull-Request [GH-353] was assigned:
https://github.com/doctrine/common/pull/353

Comment by Marco Pivetta [ 09/Feb/15 ]

Ruud Kamphuis I think single parameter is fine for now, as it is consistent and easy to handle on the closure side





[DCOM-189] Doctrine Proxies may conflict with interfaced constructors Created: 03/May/13  Updated: 24/Apr/14

Status: Reopened
Project: Doctrine Common
Component/s: None
Affects Version/s: None
Fix Version/s: 2.5.0

Type: Improvement Priority: Minor
Reporter: Harmen M Assignee: Marco Pivetta
Resolution: Unresolved Votes: 0
Labels: None


 Description   

The Doctrine ProxyGenerator generates for a proxy a constructor. The documentation of Doctrine states that the constructor is never called. For a project, I created a group of entities with a interfaced constructor in order to enforce a common interface. This results in a incompatible proxy and so a fatal error.



 Comments   
Comment by Marco Pivetta [ 03/May/13 ]

Cannot fix this - the constructor is required to override instantiation logic

Comment by Harmen M [ 03/May/13 ]

Edit: added the correct description. I accidentially submitted the form before editing the description.

Comment by Marco Pivetta [ 03/May/13 ]

Harmen M why do you have a constructor in an interface? That's a very bad practice, and it makes things quite hard to handle.

I can think of a workaround, but I first want to be sure there's a real advantage in changing the current implementation to use

unserialize()

just to handle this specific use case.

Comment by Benjamin Eberlei [ 03/May/13 ]

Adding __construct to an interface is an anti pattern and shouldn't be done.

Comment by Harmen M [ 03/May/13 ]

Ok, then I change my implementation.

But, maybe it is an idea to update the documentation of the ORM and state that constructor interfacing is not possible?
http://docs.doctrine-project.org/projects/doctrine-orm/en/latest/reference/architecture.html

Comment by Marco Pivetta [ 18/Dec/13 ]

I actually had more requests for this feature on a similar project ( https://github.com/Ocramius/ProxyManager/issues/115 ).
I will mark this as feature request, but can't guarantee that it will get into Doctrine 2.x, since it may be a BC break.

Comment by Guilherme Blanco [ 24/Apr/14 ]

Creating interfaces with __construct ties your contract preventing extensibility points. This is nature of OOP and I do not consider this should be documented because it's (in-theory) expected that people have some level of knowledge in OO design when coding.

Closing this as invalid.

Comment by Marco Pivetta [ 24/Apr/14 ]

Re-opening.

While interfaced constructors are a known bad practice, changing constructor parameters is also a well known LSP violation (a minor one).

I'll keep tracking this, but I'm blocked by HHVM's missing Closure::bind() as of https://github.com/facebook/hhvm/issues/1203





[DCOM-165] Entities seems not be recognized by AnnotationDriver Created: 02/Sep/12  Updated: 03/Dec/13

Status: Awaiting Feedback
Project: Doctrine Common
Component/s: Annotations
Affects Version/s: 2.2, 2.3
Fix Version/s: None

Type: Bug Priority: Trivial
Reporter: Maarten de Keizer Assignee: Marco Pivetta
Resolution: Unresolved Votes: 0
Labels: annotationdriver, realpath, symlink, windows
Environment:

Windows 7 Profesional, Enterprise and Windows Server 2003 and 2008; Common: 2.2.3 DBAL: 2.2.2 ORM: 2.2.3; PHP 5.4.5 and 5.3.5



 Description   

Problem:
--------
"$em->getMetadataFactory()->getAllMetadata()"
Will result in an empty array. The entitiy manager is correctly configured and the entities files are loaded (echo's in the files will be displayed).

Debug steps:
------------
Looking in the code for the problem I created the following debug points first:

file AnnotationDriver.php method getAllClassNames()
after the line: "$includedFiles[] = $sourceFile;"
add: "echo $sourceFile . ' is loaded' . PHP_EOL;"

file AnnotationDriver.php method getAllClassNames()
after the line: "$sourceFile = $rc->getFileName();"
add: "echo $className . ' is loaded from: ' . $sourceFile . ' in the includedFiles array? ' . (in_array($sourceFile, $includedFiles) ? 'YES' : 'not found ') . PHP_EOL;"

the following output will be displayed:
...
f:\workspace\nl.markei.posto-2.3\lib\relaties\Contact.php is loaded
...
nl\markei\posto\relaties\Contact is loaded from: F:\workspace\nl.markei.posto-2.3\lib\relaties\Contact.php in the includedFiles array? not found

It seems that Doctrine includes the file from f: but ReflectionClass say it is loaded from F:. The in_array() will fail and Doctrine will not recognized the entity.

But this is not the full problem. I created a new debug point:

file AnnotationDriver.php method getAllClassNames()
after the line: "foreach ($this->_paths as $path) {"
add: "echo 'path: ' . $path . ' after realpath: ' . realpath($path) . PHP_EOL;"

This will result in the following output:
"path: F:\domains\markei.nl\lib\nl.markei.posto\ after realpath: f:\workspace\nl.markei.posto-2.3\lib"

So the conversion of the F: to f: is done by realpath; its look like

A simple fix should be in AnnotationDriver.php / getAllClassNames()
replace: "$sourceFile = $rc->getFileName();"
by: "$sourceFile = realpath($rc->getFileName());"

After I did that, the problem still exists. So I add to echo's (one with realpath and one without at the part of the code). And both echo's result in a path starting with "F:".

So my first reaction was freaky!

After some frustrating hours I found the problem in the symlink I used.
"F:\domains\markei.nl\lib\nl.markei.posto" was a symlink to "f:\workspace\nl.markei.posto-2.3\lib" and realpath will not convert the driver char in the target of the symlink for some freaky reason. After changing the symlink target to "F:\workspace...." everything works fine.

Summary:
--------
realpath results are not consistent on Windows. This will resulting in problems under Windows.

Possible solutions:
-------------------
Saying "this is a PHP bug".
It is possible that there are more case sensitive problems hidding here on Windows. Maybe it is better to use (for Windows only!) a case insensitive in_array-alternative in AnnotationDriver.php getAllClassNames() (or convert all paths for to lowercase for Windows)



 Comments   
Comment by Marco Pivetta [ 23/Jan/13 ]

Maarten de Keizer looks like the issue is still there in doctrine/annotations. Are you able to come up with a failing test case?





Generated at Mon Mar 02 11:15:16 UTC 2015 using JIRA 6.2.3#6260-sha1:63ef1d6dac3f4f4d7db4c1effd405ba38ccdc558.