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

[PersistentCollection] Fix compatibility with PHP7 return types #1532

Closed
wants to merge 1 commit into from

Conversation

Maks3w
Copy link
Contributor

@Maks3w Maks3w commented Dec 7, 2016

Solves the issue about signature mismatch due missing return type in child class.

Solves the issue about signature mismatch due missing return type in child class.
@Maks3w
Copy link
Contributor Author

Maks3w commented Dec 7, 2016

I've tested in local tuning CustomCollectionsTest successfully. I didn't commit the changes due PHP5 vs PHP7 runs.

This has been tested with PHP 7.0 return types. Not with PHP 7.1 void or nullable return types

@alcaeus
Copy link
Member

alcaeus commented Dec 7, 2016

Great, I haven't even thought of that! With PHP 7.1 released, could you add support for nullable types and the void return type?

@Maks3w
Copy link
Contributor Author

Maks3w commented Dec 7, 2016

@alcaeus The code is based on Proxy code present at doctrine/commons. When that code become updated this could be done as well.

@alcaeus
Copy link
Member

alcaeus commented Dec 7, 2016

This was added to doctrine/common in 2.7.0: doctrine/common#734

@Maks3w
Copy link
Contributor Author

Maks3w commented Dec 7, 2016

Then the code proposed on this PR include those changes. I just copy the code present on master yesterday.

@alcaeus
Copy link
Member

alcaeus commented Dec 7, 2016

Ok. I'll take a look later today.

@alcaeus alcaeus added this to the 1.2 milestone Dec 7, 2016
@alcaeus alcaeus added the Feature label Dec 7, 2016
private function formatType(
\ReflectionType $type,
\ReflectionMethod $method,
\ReflectionParameter $parameter = null
Copy link
Member

Choose a reason for hiding this comment

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

We could simplify method by getting rid of this argument

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not my intention make something different from the code already present on doctrine commons.

If both snippets are equals then maintenance will be easy until a long term solution (DRY) become

Copy link
Member

Choose a reason for hiding this comment

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

That is good point. This is not-blocking comment so I'll let @alcaeus decide :)

Copy link
Member

Choose a reason for hiding this comment

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

I agree, we should try to keep it consistent.

private function formatType(
\ReflectionType $type,
\ReflectionMethod $method,
\ReflectionParameter $parameter = null
Copy link
Member

Choose a reason for hiding this comment

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

I agree, we should try to keep it consistent.

}
if ( ! $type->isBuiltin() && ! class_exists($name) && ! interface_exists($name)) {
if (null !== $parameter) {
throw UnexpectedValueException::invalidParameterTypeHint(
Copy link
Member

Choose a reason for hiding this comment

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

This should not be a Proxy\UnexpectedValueException. Either create a new exception class or throw a PersistentCollectionException.

@alcaeus
Copy link
Member

alcaeus commented Dec 8, 2016

@malarzm I was going to ask for some tests, then I saw that we currently don't have any. Would it be worth to create some tests for this class?

@malarzm
Copy link
Member

malarzm commented Dec 8, 2016

IIRC I wasn't originally writing tests for this specific class because for any of custom collection test this generator must be well and alive :)

@Maks3w
Copy link
Contributor Author

Maks3w commented Dec 8, 2016

This test class (https://github.com/doctrine/mongodb-odm/blob/d728cc1e309475ed52f4cd9aa35208e699941f9e/tests/Doctrine/ODM/MongoDB/Tests/Functional/CustomCollectionsTest.php) cover the scenario for PHP <7 but it's heavily coupled to the whole MongoDB setup and I don't have time for to perform a refactor and decouple the tests.

Sorry for not to be more useful but don't expect more changes from my side. The branch is open so any of you can add more commits

@malarzm
Copy link
Member

malarzm commented Dec 8, 2016

Sorry for not to be more useful but don't expect more changes from my side.

@Maks3w mind changing at least the exception to PersistentCollectionException so we could merge this as is and take care of tests on our own later?

@malarzm
Copy link
Member

malarzm commented Dec 27, 2016

Closing in favour of #1543. Thanks for getting us started on this @Maks3w!

@malarzm malarzm closed this Dec 27, 2016
@Maks3w Maks3w deleted the php70-return-types branch December 28, 2016 12:37
@malarzm malarzm removed this from the 1.2 milestone Jan 5, 2017
@malarzm malarzm removed the Feature label Jan 5, 2017
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.

None yet

3 participants