Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Allow Travis CI to build on PHP 7.4#32289

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
nicolas-grekas merged 1 commit intosymfony:3.4fromphansys:php74
Jul 31, 2019
Merged

Conversation

@phansys
Copy link
Contributor

@phansysphansys commentedJun 29, 2019•
edited
Loading

QA
Branch?3.4
Bug fix?no
New feature?no
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed ticketsn/a
LicenseMIT
Doc PRn/a

@Tobion
Copy link
Contributor

This needs to target 3.4

phansys and andreybolonin reacted with thumbs up emoji

@phansysphansys changed the base branch from4.4 to3.4June 29, 2019 23:19
@phansys
Copy link
ContributorAuthor

This needs to target 3.4

Target updated 👍

Copy link
Member

@nicolas-grekasnicolas-grekas left a comment•
edited
Loading

Choose a reason for hiding this comment

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

I'm putting this on hold because we usually don't add "allowed_job" lines: this would consume a job for every PR+merge, while they are quite precious.
Of course, once it's green and reasonably stable, we'll merge as a regular job.
Then, we'll want to rotate all jobs: deps=low should be the one running 7.4, deps=high 7.3, etc.
Could you please have a look to make this work?

phansys reacted with thumbs up emoji
@phansys
Copy link
ContributorAuthor

Of course!
I'll try to check how to fix the job, since the issue seems to be APC related (ref.).

@phansys
Copy link
ContributorAuthor

The tests are running now. The failures are caused by unsilenced deprecation notices.

@nicolas-grekas
Copy link
Member

Function ReflectionType::__toString() is deprecated

this happened to Twig today too andwas fixed by removing the use of some mocks
Not sure we can do it for Symfony.
Instead, we could try making thephpunit script (see repo root) use phpunit 8.2 on PHP 7.4.

@nicolas-grekas
Copy link
Member

We should mute this specific deprecation in the bridge I think, because many projects will be affected. /cc@greg0ire

phansys reacted with thumbs up emoji

@greg0ire
Copy link
Contributor

greg0ire commentedJul 1, 2019•
edited
Loading

We should mute this specific deprecation in the bridge I think, because many projects will be affected. /cc@greg0ire

🤔 maybe we could add a new deprecation group calledmuted and have a way to display them anyway, just in case?SYMFONY_DEPRECATIONS_HELPER="scream=true"? I wonder if this use case will happen a lot.

@phansys
Copy link
ContributorAuthor

PHPUnit ^8.0 seems to require some missing return declarations at inherited methods, by instance:

Declaration of Symfony\Bundle\WebProfilerBundle\Tests\DependencyInjection\WebProfilerExtensionTest::setUp() must be compatible with PHPUnit\Framework\TestCase::setUp(): void

Given that, I think this version bump can't be done in a minor release. Please, let me know how to proceed if you have elaborated some concerns about this.

@greg0ire
Copy link
Contributor

Given that, I think this version bump can't be done in a minor release.

You might be able to, but it will take some black magic:

if (class_exists('PHPUnit_Runner_Version') &&version_compare(\PHPUnit_Runner_Version::id(),'6.0.0','<')) {
class_alias('Symfony\Bridge\PhpUnit\Legacy\CoverageListenerForV5','Symfony\Bridge\PhpUnit\CoverageListener');
}elseif (version_compare(\PHPUnit\Runner\Version::id(),'7.0.0','<')) {
class_alias('Symfony\Bridge\PhpUnit\Legacy\CoverageListenerForV6','Symfony\Bridge\PhpUnit\CoverageListener');
}else {
class_alias('Symfony\Bridge\PhpUnit\Legacy\CoverageListenerForV7','Symfony\Bridge\PhpUnit\CoverageListener');
}

@greg0ire
Copy link
Contributor

I built a thing! I'm a bit unsure if this should be considered bugfix or feature, and what version of the phpunit bridge is used to test 3.4 . Please advise.

@nicolas-grekas
Copy link
Member

@greg0ire sorry I should have been more specific: we should mute based on the message AND the stack trace: only deprecations from phpunit should be muted.

@greg0ire
Copy link
Contributor

Oh ok it makes a lot more sense now ^^

@greg0ire
Copy link
Contributor

I ended up picking 4.3, see link above.

nicolas-grekas added a commit that referenced this pull requestJul 18, 2019
…greg0ire)This PR was merged into the 4.3 branch.Discussion----------[PHPUnitBridge] Mute deprecations triggered from phpunit| Q             | A| ------------- | ---| Branch?       | 4.3| Bug fix?      | yes| New feature?  | no| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets | n/a| License       | MIT| Doc PR        | n/aRequest by@nicolas-grekas here:#32289 (comment)Commits-------a4ab13d Mute deprecations triggered from phpunit
@phansys
Copy link
ContributorAuthor

@greg0ire, here is a sample build on4.3 after the merge of#32443:https://travis-ci.org/phansys/symfony/jobs/560515287.

@nicolas-grekas
Copy link
Member

Needsegulias/EmailValidator#203 now

@greg0ire
Copy link
Contributor

greg0ire commentedJul 19, 2019•
edited
Loading

@greg0ire, here is a sample build on 4.3 after the merge of#32443:https://travis-ci.org/phansys/symfony/jobs/560515287.

Oh wow my fix works already? I thought it would be necessary to have it land in thevendor directory, which implies a new release… plus you are targetting 3.4, so I really don't get it 🤔 Anyway, cool :)

@nicolas-grekas
Copy link
Member

nicolas-grekas commentedJul 19, 2019•
edited
Loading

Next step unlocked. We could replace all setup/teardown methods by before/after ones, with the same as annotations.

Copy link
Member

@nicolas-grekasnicolas-grekas left a comment

Choose a reason for hiding this comment

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

I'm now merging this on 3.4 only so that we can have some visibility on PHP7.4 support without consuming too many jobs.

@nicolas-grekas
Copy link
Member

Thank you@phansys.

phansys and greg0ire reacted with hooray emoji

@nicolas-grekasnicolas-grekas merged commit1ca61d3 intosymfony:3.4Jul 31, 2019
nicolas-grekas added a commit that referenced this pull requestJul 31, 2019
This PR was merged into the 3.4 branch.Discussion----------Allow Travis CI to build on PHP 7.4| Q             | A| ------------- | ---| Branch?       | 3.4| Bug fix?      | no| New feature?  | no| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets | n/a| License       | MIT| Doc PR        | n/aCommits-------1ca61d3 Allow Travis CI to build on PHP 7.4
@phansysphansys deleted the php74 branchJuly 31, 2019 17:03
@nicolas-grekasnicolas-grekas mentioned this pull requestJul 31, 2019
23 tasks
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@fabpotfabpotfabpot left review comments

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

Assignees

No one assigned

Projects

None yet

Milestone

3.4

Development

Successfully merging this pull request may close these issues.

6 participants

@phansys@Tobion@nicolas-grekas@greg0ire@fabpot@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp