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

Allow installation of ocramius/proxy-manager ^2.0 #1880

Merged
merged 1 commit into from Jan 20, 2017

Conversation

emodric
Copy link
Contributor

@emodric emodric commented Jan 12, 2017

While testing JMS Translation Bundle 2.0 support with Twig 2.1, I got an exception Cannot bind closure to scope of internal class stdClass. It turns out this is due to older version of ocramius/proxy-manager which does not support PHP 7 correctly.

There is no breaking changes in kernel with Proxy Manager 2.x as far as I can see.

While testing JMS Translation Bundle 2.0 support with Twig 2.1, I got an exception "Cannot bind closure to scope of internal class stdClass". It turns out this is due older version of `ocramius/proxy-manager` which does not support PHP 7 correctly.

There is no breaking changes in kernel with Proxy Manager 2.x as far as I can see.
@emodric
Copy link
Contributor Author

emodric commented Jan 12, 2017

Failure is related to Solr.

@andrerom
Copy link
Contributor

Before we merge this we should make sure tags for ezplatform is always generated on php 5.6, to avoid packages like these being locked in php 7.0 version breaking php 5.6 install.

(side concept of composer.lock essentially broken when it comes to these scenarios)

Copy link
Contributor

@andrerom andrerom left a comment

Choose a reason for hiding this comment

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

Approved besides comment above.

@emodric
Copy link
Contributor Author

emodric commented Jan 14, 2017

@andrerom Do you want a counterpart PR to ezsystems/ezplatform that adds the following to composer.json ?

"platform": {
    "php": "5.6"
}

@andrerom
Copy link
Contributor

andrerom commented Jan 16, 2017

well.. That would be needed when we generate the lock, but should ideally not be in there (composer.json) when people download the code and make it into theirs. So I mentioned it more for @bdunogier @yannickroger @lserwatka and @adib78, but we'll try to find a way for that separately from this PR. so my approval stands :)

The safest is that we just make sure to generate the tag on php 5.6 (afaik we do this atm as stated below), alternative is temporary changing composer.json to have the platform set during lock generation, then remove that and regenerating lock checksum afterwards.

@bdunogier
Copy link
Member

@andrerom Do you want a counterpart PR to ezsystems/ezplatform that adds the following to composer.json ?

I don't think we can do that, can we ? It would force php 5.6 packages on php 7.0 users even when using composer update. At least now, you can run composer update, and gets dependencies calculated for your platform.

@bdunogier
Copy link
Member

bdunogier commented Jan 16, 2017

Before we merge this we should make sure tags for ezplatform is always generated on php 5.6, to avoid packages like these being locked in php 7.0 version breaking php 5.6 install.

Unless I am mistaken, it is part of our procedures at the moment, but I'll check.

@andrerom
Copy link
Contributor

@bdunogier I think it is, do you also approve this?

@andrerom
Copy link
Contributor

Merging, but I guess it won't be much useful until 45a7a17 is reverted once JMS has fixed incompatibility with Twig 2.x.

@andrerom andrerom merged commit 9144c96 into ezsystems:6.7 Jan 20, 2017
@emodric
Copy link
Contributor Author

emodric commented Jan 20, 2017

Yep, I'm aware :) Thanks though :)

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