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

Add support for php 7.4 #1236

Merged
merged 4 commits into from
Jun 2, 2020
Merged

Add support for php 7.4 #1236

merged 4 commits into from
Jun 2, 2020

Conversation

snapshotpl
Copy link
Contributor

No description provided.

.travis.yml Outdated
before_install:
- phpenv config-rm xdebug.ini
- php: 7.4snapshot
env: SYMFONY_VERSION='^3.4'
Copy link
Member

Choose a reason for hiding this comment

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

no need for these 2 jobs testing 7.4 against specific SF versions. A single job running PHP 7.4 is enough (and we only need a single job forcing specifc SF LTS versions)

.travis.yml Outdated
@@ -26,9 +26,10 @@ matrix:
env: SYMFONY_VERSION='^3.4'
- php: 7.3
env: SYMFONY_VERSION='^4.2'

before_install:
- phpenv config-rm xdebug.ini
Copy link
Member

Choose a reason for hiding this comment

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

this should be kept for versions having xdebug. Change it to phpenv config-rm xdebug.ini || true to avoid making it fail when xdebug is not available.

@snapshotpl
Copy link
Contributor Author

Looks like Transliterator's dependecy is not ready for 7.4

@stof
Copy link
Member

stof commented Sep 30, 2019

@snapshotpl any chance to contribute there first then ?

@snapshotpl
Copy link
Contributor Author

@stof Done Behat/Transliterator#26

@snapshotpl
Copy link
Contributor Author

Waiting for v1.3 of behat/transliterator appear on packagist https://packagist.org/packages/behat/transliterator

@ciaranmcnulty
Copy link
Contributor

@snapshotpl The transliterator tag is released now on packagist

@ciaranmcnulty
Copy link
Contributor

(we don't need 'snapshot' now I think)

@snapshotpl
Copy link
Contributor Author

One test in php 7.4 doesn't pass. @ciaranmcnulty can you help me to resolve this https://travis-ci.org/Behat/Behat/jobs/637319029#L316 ?

@ciaranmcnulty
Copy link
Contributor

Hm, something must have changed around error handling...

@Sam-Burns
Copy link
Contributor

@ciaranmcnulty @snapshotpl we really need to delete the offending test at https://github.com/snapshotpl/Behat/blob/ea89bd18f290a26c0c7fc0e8aa3205242bf7e5b8/features/junit_format.feature#L529-L587 .

It's tagged only to run in PHP 5.3 and 5.4. The system by which these tags are used in Travis is a bit of a mess because:

  • Detection of whether we're in PHP 5.4 is done using the minor version only, hence problems in PHP 7.4.
  • This problem didn't come up with PHP 7.3 because of a missing decimal point, meaning it was trying to run tests tagged as php53 but not php5.3.

I'll do a separate pull request cleaning up the whole fiasco at

- echo "<?php echo '@php5' . implode(',@php5.', range(3, PHP_MINOR_VERSION));" > php_version_tags.php
, but deleting this test is definitely necessary as it is tagged only to run in PHP versions we don't even seem to be testing in anymore.

The rest of your PHP looks great, thanks so much for doing this.

@snapshotpl
Copy link
Contributor Author

@Sam-Burns nice catch! So I'm waiting for PR to clean this an then I will rebase your fix here

@Sam-Burns
Copy link
Contributor

Regarding the Appveyor and Travis builds: I think if we merge (or you rebase on top of) #1273 and #1270, this PR should be ready to merge.

@snapshotpl
Copy link
Contributor Author

@Sam-Burns awesome. Now waiting for fixed builds. @ciaranmcnulty ?

@snapshotpl
Copy link
Contributor Author

@ciaranmcnulty ready to merge!

@pamil pamil merged commit 49d1a83 into Behat:master Jun 2, 2020
@pamil
Copy link
Member

pamil commented Jun 2, 2020

Thanks, Witold! 🎉

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.

5 participants