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

Use more mocking instead of allowing to override final classes #13

Merged
merged 2 commits into from
Nov 22, 2022

Conversation

carlobeltrame
Copy link

If a package vendor specifies a class as final, they really do not expect the class to ever be overridden. Also, adding this package to require-dev allows us to do so in a dev environment, even accidentally outside of tests, and that code will only fail when running in production.
Instead of trying to enable mocking SearchFilter, we can just instantiate it and mock a little more in another place. The test testNormalizeDoesntReplaceWhenStrategyIsNotExact was flawed, because the search strategy is always forced to be "exact" when filtering by an association. For this reason, I removed that test, because it does not make sense to test a primitive field filter in the context of RelatedCollectionLinkNormalizer. This normalizer is only concerned with associations, not with primitive fields.

If a package vendor specifies a class as final, they really do not
expect the class to ever be overridden. Also, adding this package
to require-dev allows us to do so in a dev environment, even
accidentally outside of tests, and that code will only fail when
running in production.
Instead of trying to enable mocking SearchFilter, we can just
instantiate it and mock a little more in another place.
The test testNormalizeDoesntReplaceWhenStrategyIsNotExact was flawed,
because the search strategy is always forced to be "exact" when
filtering by an association. For this reason, I removed that test,
because it does not make sense to test a primitive field filter in the
context of RelatedCollectionLinkNormalizer. This normalizer is only
concerned with associations, not with primitive fields.
@usu
Copy link
Owner

usu commented Nov 22, 2022

I can see the potential danger, so if we find good alternatives, good for me.

Also, adding this package to require-dev allows us to do so in a dev environment, even accidentally outside of tests

The package is only activated in the test bootstrap, not in dev environment. But yeah, theoretically, if you extend a final class and only look a the test results, they could run fine even though the dev & prod environment would not run. My expectation would be though, that both psalm & phpstan would scream about this. Haven't tested though.

Currently, the following test is still broken:
89) App\Tests\Serializer\Normalizer\RelatedCollectionLinkNormalizerTest::testNormalizeDoesntReplaceWhenFilterIsNotSearchFilter
PHPUnit\Framework\MockObject\ClassIsFinalException: Class "ApiPlatform\Doctrine\Orm\Filter\DateFilter" is declared "final" and cannot be doubled

@carlobeltrame
Copy link
Author

Currently, the following test is still broken: 89) App\Tests\Serializer\Normalizer\RelatedCollectionLinkNormalizerTest::testNormalizeDoesntReplaceWhenFilterIsNotSearchFilter PHPUnit\Framework\MockObject\ClassIsFinalException: Class "ApiPlatform\Doctrine\Orm\Filter\DateFilter" is declared "final" and cannot be doubled

Thanks, fixed

@usu usu merged commit 6c04fa2 into usu:chore/api-platform-3.0 Nov 22, 2022
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.

2 participants