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

Make Sylius compatible with PHP 7.2 #8771

Merged
merged 2 commits into from
Oct 17, 2017
Merged

Conversation

stefandoorn
Copy link
Contributor

@stefandoorn stefandoorn commented Oct 4, 2017

Q A
Branch? 1.0
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
License MIT

Run test suite already against PHP 7.2 to pro-actively make adjustments before PHP 7.2 is released (if possible). Not sure if this should be against 1.0 or master though ;-)

@stefandoorn
Copy link
Contributor Author

First issue spotted: friendsofphp/php-cs-fixer needs to be at 2.6.1 at least to support PHP 7.2.

@stefandoorn stefandoorn changed the title Test suite against PHP 7.2 [WIP] Test suite against PHP 7.2 Oct 4, 2017
@stefandoorn
Copy link
Contributor Author

I'm not sure how to figure out what the Behat errors are. Output seems scarce.

@gabiudrescu
Copy link
Contributor

@stefandoorn if you check this etc/travis/run-suite after_failure "${SYLIUS_SUITE}" the build log, you can reach this:

http://termbin.com/uw75

which is pretty verbose.

@stefandoorn
Copy link
Contributor Author

Ok..

idn_to_ascii(): INTL_IDNA_VARIANT_2003 is deprecated in /home/travis/build/Sylius/Sylius/vendor/league/uri/src/Components/HostnameTrait.php line 68

league/uri is required by Payum at ~4.0. Not sure about it, but issue might be fixed in league/uri 5.x. Doesn't seem Payum already supports this, so this might be a hard path to get fixed.

@stefandoorn
Copy link
Contributor Author

Anyone also knows the URL on which the imgur uploaded screenshots can be found?

@pamil
Copy link
Contributor

pamil commented Oct 9, 2017

@stefandoorn looks like our imgur API key is not valid so uploading screenshots has stopped working.

.travis.yml Outdated
- memcached
-
sudo: required
php: nightly
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd skip the nightly PHP version for now as there's still some time until PHP 7.3 gets released & builds will take more time to finish.

Copy link
Contributor

Choose a reason for hiding this comment

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

Instead, running packages build on PHP 7.2 is a must :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mainly added nightly for now as it might discover some issues now already that we can tackle directly. Will take it out before finalising PR.

composer.json Outdated
@@ -88,7 +88,7 @@
"matthiasnoback/symfony-dependency-injection-test": "^1.0",
"mikey179/vfsStream": "^1.6",
"pamil/prophecy-common": "^0.1",
"phpspec/phpspec": "^4.0",
"phpspec/phpspec": "^4.0@dev",
Copy link
Contributor

Choose a reason for hiding this comment

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

Same change applies for every Sylius package using phpspec (packages build on PHP 7.2 would fail without this one).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright. Need to see if this change is required at all before finalising PR.

@pamil
Copy link
Contributor

pamil commented Oct 10, 2017

Not sure if this should be against 1.0 or master though ;-)

IMO it should go for 1.0 branch as 1.0 requires PHP ^7.1, so PHP 7.2 should also be supported there. (rebase needed though then)

@pamil pamil changed the base branch from master to 1.0 October 10, 2017 09:22
@pamil
Copy link
Contributor

pamil commented Oct 10, 2017

As for that Payum-related errors:

  • Payum itself requires PHP 5.5+
  • Payum requires thephpleague/uri v4:
    • thephpleague/uri v4 requires PHP 5.5+
    • thephpleague/uri v4 uses the deprecated function call
    • thephpleague/uri v4 requires jeremykendall/php-domain-parser v3:
      • jeremykendall/php-domain-parser v3 uses the deprecated function call
  • If Payum requires thephpleague/uri v5:
    • thephpleague/uri v5 requires PHP 7.0+
    • thephpleague/uri v5 requires jeremykendall/php-domain-parser v4.0.3-alpha:
      • jeremykendall/php-domain-parser v4.0.3-alpha does not use the deprecated function call

Conclusion:

@pamil
Copy link
Contributor

pamil commented Oct 10, 2017

jeremykendall/php-domain-parser#204 and thephpleague/uri-src#41 have been opened, let's wait for the replies.

@pamil pamil added this to the 1.0 milestone Oct 17, 2017
@pamil pamil added Maintenance CI configurations, READMEs, releases, etc. PHP 7.2 labels Oct 17, 2017
@stefandoorn stefandoorn changed the title [WIP] Test suite against PHP 7.2 Make Sylius compatible with PHP 7.2 Oct 17, 2017
@stefandoorn
Copy link
Contributor Author

@pamil Ready to review. Had to explicitly require the updated dependencies to get the proper versions installed.

Build on PHP 7.3 fails due to PHP-CS-Fixer not allowing 7.3. I'm not sure we should disallow installation on PHP 7.3 due to this.

Copy link
Contributor

@pamil pamil left a comment

Choose a reason for hiding this comment

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

Awesome! Can you add packages build for PHP 7.2? It's like the one for PHP 7.1 but with docs removed from $SYLIUS_SUITE 🎉

@pamil pamil merged commit 153e380 into Sylius:1.0 Oct 17, 2017
@pamil
Copy link
Contributor

pamil commented Oct 17, 2017

Thanks Stefan, great job! 🥇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Maintenance CI configurations, READMEs, releases, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants