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

Fixed deprecation with Symfony 4.2 #164

Merged
merged 9 commits into from
Dec 15, 2018
Merged

Fixed deprecation with Symfony 4.2 #164

merged 9 commits into from
Dec 15, 2018

Conversation

jmsche
Copy link
Contributor

@jmsche jmsche commented Dec 5, 2018

Q A
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Fixed tickets closes #167
License MIT

Hi, this PR fixes a small deprecation introduced in Symfony 4.2; the fix includes a BC layer for older versions.

@kunicmarko20
Copy link
Contributor

Had to do the same thing, thank you! Can you maybe check the build errors?

@jmsche
Copy link
Contributor Author

jmsche commented Dec 5, 2018

I'm no Travis expert, but I tried to fix the DI component to version 4.2 for the phpstan check - seems it breaks memory limit (which is set to 1.5GB).

Should I increase composer's memory limit for this build? Do you think this issue is "normal"?

@kunicmarko20
Copy link
Contributor

Not a normal issue, will check it out tonight

@jmsche
Copy link
Contributor Author

jmsche commented Dec 10, 2018

Just pushed a commit increasing the memory limit of composer - for this check - reveals the sensio/distribution-bundle dependency can't be installed as it requires DI < 4.

Also, an other failing test, but I think it is unrelated to what I did.

Any idea how I can fix this dependency issue?

@kunicmarko20
Copy link
Contributor

Accidentally closed, didn't have time to check this and now I am afk for a few days. Do we event need distribution bundle?

About test, took a quick look and it seems that new version of phpunit has a different signature which is a BC break, not sure tho

@jmsche
Copy link
Contributor Author

jmsche commented Dec 10, 2018

Do we event need distribution bundle?

For Symfony < 4 I guess. It shouldn't install if Flex is already there.

@kunicmarko20
Copy link
Contributor

Do we use it in code? If no, drop it, sf should install it as its dependency

@kunicmarko20
Copy link
Contributor

Update symfony phpunit bridge to 4.1 that should solve it IIRC.

Also, do we still need memory limit?

@jmsche
Copy link
Contributor Author

jmsche commented Dec 10, 2018

I removed composer memory_limit env var as it's not required anymore - needed it to get the error message from composer.

Well, it only gets worse :p I changed the min requirement of phpunit bridge to ^4.1, now two tests fail.

@@ -24,8 +24,14 @@
public function getConfigTreeBuilder(): TreeBuilder
{
$treeBuilder = $this->createTreeBuilder();
$treeBuilder
->root('fos_ck_editor')
if (method_exists($treeBuilder, 'getRootNode')) {
Copy link

@angelsk angelsk Dec 14, 2018

Choose a reason for hiding this comment

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

Try this instead, because TreeBuilder has no constructor in previous versions

Suggested change
if (method_exists($treeBuilder, 'getRootNode')) {
if (method_exists(TreeBuilder::class, 'getRootNode')) {
$treeBuilder = new TreeBuilder('fos_ck_editor');
$rootNode = $treeBuilder->getRootNode();
} else {
$treeBuilder = new TreeBuilder();
$rootNode = $treeBuilder->root('fos_ck_editor');
}

@angelsk
Copy link

angelsk commented Dec 14, 2018

Try adding - SYMFONY_PHPUNIT_VERSION="6.5" to the test block that's failing

@kunicmarko20
Copy link
Contributor

kunicmarko20 commented Dec 15, 2018

Green 🎉 now to address suggestions by @angelsk

@kunicmarko20
Copy link
Contributor

Should be good now, thank you @jmsche and @angelsk!

@kunicmarko20 kunicmarko20 merged commit 6f05079 into FriendsOfSymfony:2.x Dec 15, 2018
@kunicmarko20
Copy link
Contributor

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.

Deprecations with Symfony 4.2
3 participants