Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proxy: Added support for PHP 7 return & parameter type hints #376

Closed
wants to merge 1 commit into from

Conversation

Majkl578
Copy link
Contributor

This PR adds support for PHP 7 return types and parameter type hints on generated proxies.

@doctrinebot
Copy link

Hello,

thank you for creating this pull request. I have automatically opened an issue
on our Jira Bug Tracker for you. See the issue link:

http://www.doctrine-project.org/jira/browse/DCOM-294

We use Jira to track the state of pull requests and the versions they got
included in.

@@ -777,6 +777,22 @@ private function generateMethods(ClassMetadata $class)
}

$methods .= $name . '(' . $this->buildParametersString($class, $method, $method->getParameters()) . ')';

if (PHP_VERSION_ID >= 70000 && $method->hasReturnType()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be split into a private method?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@TomasVotruba
Copy link
Contributor

👍

$this->markTestSkipped('Method return types are only supported in PHP >= 7.0.0.');
}

$className = 'Doctrine\Tests\Common\Proxy\ReturnTypesClass';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can use PHP 5.6 ::class, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, that is a syntax error on PHP <5.5 (and this package is >=5.3.2).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, just checked.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would fail at parse time, fyi ;-)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realized, to bad :/

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gonna change it anyway: bumping the library dependencies.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, let me know if/when you need rebase or other changes here.

@Majkl578
Copy link
Contributor Author

@Ocramius: Is there anything else that needs to be done?

@Ocramius Ocramius self-assigned this Aug 31, 2015
@Ocramius
Copy link
Member

@Majkl578 no, this looks good, but the patch has to wait for 2.6.0, so I'll first have to branch out 2.5.x to keep it stable.

@Majkl578
Copy link
Contributor Author

@Ocramius: Makes sense. 👍

(Hoping that 2.6 will be released when PHP 7 reaches stable release. :) )

@Ocramius
Copy link
Member

Ocramius commented Dec 3, 2015

Can haz PHP 7 support plz? Yes, we can, thanks to @Majkl578 :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants