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

Adding Symfony4 support #171

Closed
wants to merge 9 commits into from
Closed

Adding Symfony4 support #171

wants to merge 9 commits into from

Conversation

geecu
Copy link

@geecu geecu commented Nov 18, 2017

#SymfonyConHackday2017

Gunther Konig added 8 commits November 18, 2017 12:07
v4 doesn't generate correct mocks for 7.1 classes

Tests are failing with:
Fatal error: Declaration of Mock_ContainerBuilder_15214828::getReflectionClass(string $class, bool $throw = true) must be compatible with Symfony\Component\DependencyInjection\ContainerBuilder::getReflectionClass(?string $class, bool $throw = true): ?ReflectionClass
phpunit 5.4.0 deprecated `PHPUnit\Framework\TestCase::getMock()`
Replace its usages with the recommended `createMock()` where possible,
or with `getMockBuilder()` were `createMock()`'s defaults won't do
for compatibility with php 5.5
Tests are ran with `phpunit@4.5` for `php5.5` and `phpunit@5.7` for
all the others. To have the tests working in both versions, avoid using
`PHPUnit\Framework\TestCase::createMock()` which was added in
`phpunit@5.4.0`.
composer.json Outdated
"php": "^5.3.9|^7.0",
"symfony/framework-bundle": "~2.3|~3.0",
"symfony/finder": "~2.3|~3.0",
"php": "^5.3.9|^7.0|^7.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

this change should not be necessary afaik

Copy link
Author

Choose a reason for hiding this comment

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

you're right, I removed |^7.1

@lsmith77
Copy link
Contributor

awesome work .. I restarted the one test that failed due to composer manually and now its all green

@geecu
Copy link
Author

geecu commented Nov 20, 2017

Please note that the bundle needs more work to make it work in a Symfony Flex directory structure. I've spent some time during the hackday trying to understand what I should update, without too much success. I think keeping compatibility with both <3.4and Flex apps is going to be quite verbose, maybe you're thinking about tagging a new major version of this bundle that will be compatible only with Flex?

"symfony/console": "~2.3|~3.0",
"symfony/expression-language": "~2.6|~3.0",
"symfony/templating": "~2.3|~3.0",
"phpunit/phpunit": "~4.5|~5.7",
Copy link
Contributor

Choose a reason for hiding this comment

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

you may want to allow ^6.0 too

Copy link
Author

Choose a reason for hiding this comment

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

First I tried using ^6.0, but that phpunit version switches to using PHP namespaces and I thought it would require too many changes in the test files, this is why I stuck with 5.7.

Copy link
Contributor

Choose a reason for hiding this comment

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

Switching to the namespaced classes is probably a good idea. But could be done in a different PR IMO.

Copy link
Author

Choose a reason for hiding this comment

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

At the same time, phpunit 4.5 is required when running the tests under PHP 5.5, because phpunit 5.7 requires PHP ^5.6. So the tests extending namespaced phpunit classes wouldn't be able to run under that version. And requiring PHP^5.6 in this bundle would probably require a major version, to avoid BC...

Copy link
Contributor

Choose a reason for hiding this comment

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

You can use 4.8.35 or higher on PHP 5.5 which provides a forward compatible layer for the namespaced classes.

@geecu
Copy link
Author

geecu commented Nov 21, 2017

Also, as mentioned in a possible issue in the templating component, the templating component is now obsolete, so this bundle (maybe) shouldn't rely on it anymore. @lsmith77 , do you think I should open a new issue (followed, hopefully by a pull request) suggesting to release Symfony 4 (and Flex directory structure) compatibility in a new major version? That would implement/rely only on the twig bridge?

@geecu geecu mentioned this pull request Nov 23, 2017
@lsmith77
Copy link
Contributor

I would say .. maybe for Symfony 4 we should do a new major version which drops the dependency on the templating component.

@lsmith77
Copy link
Contributor

lsmith77 commented Dec 5, 2017

see also #173

@lsmith77 lsmith77 closed this Dec 5, 2017
@lsmith77 lsmith77 removed the in review label Dec 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