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.

Sign up for GitHub

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

Jump to bottom

PHP 7.2+ - Doctrine to 2.6.3 #1989

Closed
wants to merge 1 commit into from
Closed

PHP 7.2+ - Doctrine to 2.6.3 #1989

wants to merge 1 commit into from

Conversation

EtienneBruines
Copy link
Contributor

@EtienneBruines EtienneBruines commented Jan 22, 2019 •
edited
Loading

Let's just see if the tests run on the CI server and if any doctrine-related issues occur.

1. Why is this change necessary?

The current version of doctrine is not compatible with PHP 7.3, due to

PHP Warning:  "continue" targeting switch is equivalent to "break". Did you mean to use "continue 2"? in ./lib/Doctrine/ORM/UnitOfWork.php on line 2718

See also doctrine/orm#7325

The same goes for zendframework/zend-code. The latest version of zend-code is supported by ocramius/proxy-manager at 2.2.0, but that one requires PHP 7.2. So we require PHP 7.2 or above. 🎉

2. What does this change do, exactly?

  • php to 7.2
  • doctrine/orm to 2.6.3
  • ocramius/proxy-manager to 2.2.2

3. Describe each step to reproduce the issue or behaviour.

4. Please link to the relevant issues (if any).

5. Which documentation changes (if any) need to be made because of this PR?

6. Checklist

  • I have squashed any insignificant commits
  • This change has comments for package types, values, functions, and non-obvious lines of code
  • I have read the contribution requirements and fulfil them.

@bcremer
Copy link
Contributor

bcremer commented Jan 23, 2019 •
edited
Loading

I noticed you deleted /engine/Library/Doctrine/ORM/Persisters/Entity/BasicEntityPersister.php. I am pretty sure this file is required for Shopware to work. The file overwrites the one in the vendor directory and contains a little hack, see: https://github.com/shopware/shopware/blob/c22a29376ff5a713d70afc310c371e1faa21d77c/engine/Library/Doctrine/ORM/Persisters/Entity/BasicEntityPersister.php#L671

@shyim shyim changed the title Updated composer to 2.6.3 Updated doctrine to 2.6.3 Jan 23, 2019
@EtienneBruines
Copy link
Contributor Author

@bcremer Agreed. Back then, you apparently already put in a lot of effort; and with the new version, there's still no better solution.

I'll be updating the files, though. Coming right up.

@EtienneBruines EtienneBruines changed the title Updated doctrine to 2.6.3 Doctrine to 2.6.3; ProxyManager to 2.2.2; PHP to 7.2 Jan 23, 2019
@EtienneBruines
Copy link
Contributor Author

For future reference:

https://gitter.im/shopware/shopware?at=5c4833abc45b986d11a6e9a6

@soebbing:
Policy of Shopware is to support the oldest current actively supported PHP version - that would be PHP 7.1

I will be removing any PHP 7.2 requirement.

@EtienneBruines EtienneBruines changed the title Doctrine to 2.6.3; ProxyManager to 2.2.2; PHP to 7.2 Doctrine to 2.6.3; ZendCode hotfix Jan 23, 2019
@soebbing
Copy link
Contributor

Just here to point out: You've sparked a new internal discussion, @EtienneBruines, if it is really worth a hack to support a PHP version that will only be around for about half a year before it is deprecated.

So the last word on this hasn't been spoken, I'll get back to you soon.

@bcremer
Copy link
Contributor

bcremer commented Jan 23, 2019

PHP 7.1 is already in Security Only mode so it would make sense to support PHP 7.3.

image

@mitelg
Copy link
Contributor

mitelg commented Jan 23, 2019

my vote for minimum PHP 7.2 👍

@soebbing
Copy link
Contributor

Our prayers have been heard! Shopware 5.6 will have PHP 7.2 as a minimum requirement, @EtienneBruines!
So please please squash your commits, have a look at the CI-errors and this PR is good to go! 👍 💙

@EtienneBruines EtienneBruines changed the title Doctrine to 2.6.3; ZendCode hotfix PHP 7.2+ - Doctrine to 2.6.3 Jan 28, 2019
@EtienneBruines
Copy link
Contributor Author

EtienneBruines commented Jan 28, 2019 •
edited
Loading

I am unfortunately unable to locate the issue.

1) Shopware\Tests\Functional\Plugins\Core\MarketingAggregate\Components\AlsoBoughtTest::testInitAlsoBought
     [exec] Failed asserting that actual size 27 matches expected size 30.

#help-needed

@shopwareBot
Copy link

shopwareBot commented Jan 29, 2019 •
edited
Loading

Hey @EtienneBruines,

this seems to be an issue with our 5.6 branch at the moment, so don't worry, we're taking care of it. :-)

Yet, please squash your commits.

@EtienneBruines
Copy link
Contributor Author

It should be ready now 😄

@EtienneBruines
PHP 7.2 minimum and PHP 7.3 compat
d322780 
Updates doctrine/orm to 2.6.3 and ocramius/proxy-manager to 2.2.2.
Due to dependencies, we have to be PHP 7.2 at least.

For a test-case:
Fixed expectedException to InvalidArgument. Doctrine decided
to no longer throw its own Exception.

Within the Article Resource:
Fixed overwriting of Configurator\Group options.

It is a good thing Options get added to the Group, because the
Group or the Options might be new - but it should not overwrite
the Options that are associated with the Group, that are not
associated with the current Article.
@shopwareBot
Copy link

Hello,

thank you for creating this pull request.
I have opened an issue on our Issue Tracker for you. See the issue link: https://issues.shopware.com/issues/SW-23399

Please use this issue to track the state of your pull request.

@shyim shyim self-assigned this Feb 14, 2019
shopwareBot pushed a commit that referenced this pull request Feb 14, 2019
@EtienneBruines @shyim
SW-23399 - PHP 7.2 minimum and PHP 7.3 compat
be9cad4 
Updates doctrine/orm to 2.6.3 and ocramius/proxy-manager to 2.2.2.
Due to dependencies, we have to be PHP 7.2 at least.

For a test-case:
Fixed expectedException to InvalidArgument. Doctrine decided
to no longer throw its own Exception.

Within the Article Resource:
Fixed overwriting of Configurator\Group options.

It is a good thing Options get added to the Group, because the
Group or the Options might be new - but it should not overwrite
the Options that are associated with the Group, that are not
associated with the current Article.
fixes #1989
@shopwareBot
Copy link

Hi @EtienneBruines, thank you very much for this contribution! 🎉 💙

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Accepted
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@EtienneBruines @bcremer @soebbing @mitelg @shopwareBot @PaddyS @shyim