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

Fix support for PHP 7.2#23711

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
fabpot merged 2 commits intosymfony:2.7fromSimperfit:feature/fix-test-for-7-2
Oct 10, 2017
Merged

Conversation

@Simperfit
Copy link
Contributor

@SimperfitSimperfit commentedJul 29, 2017•
edited by nicolas-grekas
Loading

QA
Branch?2.7
Bug fix?yes
New feature?no
BC breaks?no
Deprecations?no
Tests pass?no
Fixed tickets#23671
LicenseMIT
Doc PR-

There are still the deprecation problem with phpunit since it useeach().

There are 3 tests linked to session that I don't know how to fix / what to do, do you have any idea@nicolas-grekas ?

@SimperfitSimperfit changed the base branch frommaster to2.7July 29, 2017 09:16
@SimperfitSimperfitforce-pushed thefeature/fix-test-for-7-2 branch from57d52ed toe041175CompareJuly 29, 2017 09:18
@nicolas-grekasnicolas-grekas changed the titlefeature: fix symfony test for php 7.2Fix support for PHP 7.2Jul 29, 2017
-php:7.1
env:deps=low
-php:7.2
env:deps=high

Choose a reason for hiding this comment

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

no need to add a new section, 7.1 should be changed to 7.2 just above instead, deps=low

@SimperfitSimperfitforce-pushed thefeature/fix-test-for-7-2 branch from9c44ac7 to28ae236CompareJuly 29, 2017 10:08
@SimperfitSimperfit changed the titleFix support for PHP 7.2Fix support for PHP 7.2 on 2.7Jul 29, 2017
@xabbuhxabbuh added this to the2.7 milestoneAug 1, 2017
@Simperfit
Copy link
ContributorAuthor

Simperfit commentedAug 1, 2017•
edited
Loading

@nicolas-grekas
I've got someini_set(): Headers already sent. You cannot change the session module's ini settings at this time
on the\Symfony\Component\HttpFoundation\Tests\Session\Storage\Handler\PdoSessionHandlerTest::testSessionGC

Could you give me insight on how to fixthis ?

I need to look for the seg fault too.

@SimperfitSimperfitforce-pushed thefeature/fix-test-for-7-2 branch from28ae236 tofe1e9f2CompareAugust 1, 2017 16:10
@nicolas-grekas
Copy link
Member

Dunno sorry...

@SimperfitSimperfitforce-pushed thefeature/fix-test-for-7-2 branch fromfe1e9f2 to5dfd95aCompareAugust 9, 2017 14:21
$storage->close();
$this->assertEquals(2,$pdo->query('SELECT COUNT(*) FROM sessions')->fetchColumn(),'No session pruned because gc not called');
// @TODO: Really not sure about this at all....
true ===headers_sent() &&$storage->destroy('gc_id');
Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

I guess@stof or@ogizanagi could know if there is something better then this dirty hack ?

@SimperfitSimperfitforce-pushed thefeature/fix-test-for-7-2 branch from5dfd95a to05017efCompareAugust 24, 2017 14:37

foreach ($optionsas$key =>$value) {
if (isset($validOptions[$key])) {
if (isset($validOptions[$key]) &&false ===headers_sent()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

imo you should test for(PHP_VERSION_ID < 70200 || false=== headers_sent()), in order to keep the original (even flawed) behavior...

But that may be dirty actually.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

I think it's actually a little dirty. Then we should do that in every modified component

@SimperfitSimperfitforce-pushed thefeature/fix-test-for-7-2 branch 2 times, most recently fromd3d321a to49302f5CompareAugust 31, 2017 12:08

$packages =array();
$flags = \PHP_VERSION_ID >=50400 ?JSON_PRETTY_PRINT |JSON_UNESCAPED_SLASHES |JSON_UNESCAPED_UNICODE :0;

Copy link
Member

Choose a reason for hiding this comment

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

please revert this. These lines were here for readability

.travis.yml Outdated
cp composer.json composer.json.orig &&
echo -e '{\n"require":{'"$(grep phpunit-bridge composer.json)"'"php":"*"},"minimum-stability":"dev"}' > composer.json &&
php .github/build-packages.php HEAD^ $COMPONENTS &&
(php .github/build-packages.php HEAD^ $COMPONENTS || true) &&
Copy link
Member

Choose a reason for hiding this comment

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

why ? If this fails, the build will be garbage

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Because it does trigger a fatal on git and because of that trigger a fatal on php

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

If we have a better way for this, maybe by fixing github/build-packager.php:11

$mergeBase = trim(shell_exec(sprintf('git merge-base %s HEAD', array_shift($dirs))));

that triggers a git fatal then a seg fault than aexit 139 which lead to a build failure

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

I'm trying to have the test running and we should be discussing this

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

/home/travis/.travis/job_stages: line 57: 14926 Segmentation fault      (core dumped) $COMPOSER_UPThe command "if [[ ! $skip ]]; then $COMPOSER_UP; fi" failed and exited with 139 during .

ini_set('session.save_handler','files');
ini_set('session.save_path',sys_get_temp_dir());
//ini_set('session.save_handler', 'files');
//ini_set('session.save_path', sys_get_temp_dir());
Copy link
Member

Choose a reason for hiding this comment

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

code should not be commented. If it is not needed anymore, it should be removed


foreach ($optionsas$key =>$value) {
if (isset($validOptions[$key])) {
if (isset($validOptions[$key]) &&false ===headers_sent()) {
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 not be checked for each option, but only once before the loop.
And it should use!header_sent()

$storage->open('','sid');
$storage->read('gc_id');
ini_set('session.gc_maxlifetime', -1);// test that you can set lifetime of a session after it has been read
// IN 7.2 this does not work
Copy link
Member

Choose a reason for hiding this comment

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

if this test does not work in PPH 7.2, this case should be extracted to a separate test and be skipped when unapplicable instead of skipping one step (and ensuring other things still work)

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

I agree

.travis.yml Outdated
# - php: 5.3
# - php: 5.4
#- php: 5.5
#- php: 5.6
Copy link
Member

Choose a reason for hiding this comment

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

Must be reverted

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

This will be reverted

#- php: 5.5
#- php: 5.6
-php:7.0
env:deps=high
Copy link
Member

Choose a reason for hiding this comment

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

must be reverted

#- php: 5.6
-php:7.0
env:deps=high
-php:7.1
Copy link
Member

Choose a reason for hiding this comment

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

We need to keep a PHP 7.1 job (even though it is not one doing a special handling of deps). Otherwise, Symfony would be untested on 7.1, which is not acceptable.

}
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

must be reverted

{
$request = Request::create('/');
session_name('foo');
//session_name('foo');
Copy link
Member

Choose a reason for hiding this comment

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

commenting is a no-go. If we don't want this code anymore, it should be removed.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

I know that, I think it's like you said, all of this should be skipped on 7.2

@Simperfit
Copy link
ContributorAuthor

Thanks@stof for the review ill do the changes

@SimperfitSimperfitforce-pushed thefeature/fix-test-for-7-2 branch 3 times, most recently fromeaa77c6 to5d4838bCompareSeptember 1, 2017 07:53
@Simperfit
Copy link
ContributorAuthor

Ok, I couldn't add || true on the phpunit running that would be wrong. But everything is triggering a segfault.

@remicollet maybe you have an idea ?

@stof
Copy link
Member

stof commentedSep 1, 2017

@Simperfit please avoid mentioning me in your commit message when fixing the review. This sends me a notification when pushing the commit to Github. And anytime you amend or rebase the commit, this creates a new commit, and so pushing it sends me another notification.

sstok reacted with laugh emoji

.travis.yml Outdated
-if [[ ! $skip ]]; then $COMPOSER_UP; fi
-if [[ ! $skip ]]; then ./phpunit install; fi
-if [[ ! $skip ]]; then $COMPOSER_UP || true; fi
-if [[ ! $skip ]]; then ./phpunit install || true; fi
Copy link
Member

Choose a reason for hiding this comment

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

Adding|| true is totally a no-go. If the setup of the testsuite fails, we don't want to ignore the failure and try the next step. This would just lead to an output impossible to understand due to weird failures later (trying to use the packages which were not installed properly before)

Copy link
ContributorAuthor

@SimperfitSimperfitSep 7, 2017•
edited
Loading

Choose a reason for hiding this comment

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

Do you know how we could debug this segmentation fault ? I think something needs to be done globally in the script since it does not work even adding OR true

.travis.yml Outdated
-|
# phpinfo
if [[ ! $PHP = hhvm* ]]; then php -i; else hhvm --php -r 'print_r($_SERVER);print_r(ini_get_all());'; fi
if [[ ! $PHP = hhvm* ]]; then php -i || true; else hhvm --php -r 'print_r($_SERVER);print_r(ini_get_all());'; fi
Copy link
Member

Choose a reason for hiding this comment

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

doesphp -i really fail ? If yes, it means that the PHP shipped in Travis is totally broken, and there is no hope.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

it does not fail since it shows thephpinfo(); but it seems that it triggers a segfault

{
ini_set('session.save_handler','files');
ini_set('session.save_path',sys_get_temp_dir());
if (PHP_VERSION_ID <70200) {
Copy link
Member

Choose a reason for hiding this comment

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

PHP_VERSION_ID should be fully-qualified instead of relying on constant fallback, to allow resolving the version check at compilation time.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

What do you mean ? (PHP_VERSION maybe ?)

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

whoops, understood


publicfunctiontestSessionGC()
{
if (PHP_VERSION_ID ===70200) {
Copy link
Member

Choose a reason for hiding this comment

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

this would make the test run again on 7.2.1. Are you sure about it ?

ini_set('session.save_handler','files');
ini_set('session.save_path',sys_get_temp_dir());
//ini_set('session.save_handler', 'files');
//ini_set('session.save_path', sys_get_temp_dir());
Copy link
Member

Choose a reason for hiding this comment

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

commenting code is still a no-go

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

will remove

@nicolas-grekasnicolas-grekasforce-pushed thefeature/fix-test-for-7-2 branch 3 times, most recently from5f48ccd to25f9519CompareOctober 9, 2017 11:24
nicolas-grekas added a commit that referenced this pull requestOct 9, 2017
…ethod (nicolas-grekas)This PR was merged into the 3.3 branch.Discussion----------[Bridge\PhpUnit] Fix infinite loop when running isolated method| Q             | A| ------------- | ---| Branch?       | 3.3| Bug fix?      | yes| New feature?  | no| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets | -| License       | MIT| Doc PR        | -May unlock#23003 (ping@paul-m)And required to make#23711 pass.Commits-------e07c2f1 [Bridge\PhpUnit] Fix infinite loop when running isolated method
@nicolas-grekasnicolas-grekasforce-pushed thefeature/fix-test-for-7-2 branch 8 times, most recently fromcf43c55 to306cd02CompareOctober 9, 2017 17:55
@nicolas-grekas
Copy link
Member

Tests are now green and PR is ready.

@Simperfit
Copy link
ContributorAuthor

thanks for the help !

@fabpot
Copy link
Member

Thank you@Simperfit.

@fabpotfabpot merged commitfdf285b intosymfony:2.7Oct 10, 2017
fabpot added a commit that referenced this pull requestOct 10, 2017
This PR was merged into the 2.7 branch.Discussion----------Fix support for PHP 7.2| Q             | A| ------------- | ---| Branch?       |  2.7| Bug fix?      | yes| New feature?  | no| BC breaks?    | no| Deprecations? | no| Tests pass?   | no| Fixed tickets |#23671| License       | MIT| Doc PR        | -There are still the deprecation problem with phpunit since it use `each()`.There are 3 tests linked to session that I don't know how to fix / what to do, do you have any idea@nicolas-grekas ?Commits-------fdf285b Fix 7.2 compat layere229dd0 Fix PHP 7.2 support
This was referencedNov 10, 2017
}

if (headers_sent($file,$line)) {
thrownew \RuntimeException(sprintf('Failed to set the session handler because headers have already been sent by "%s" at line %d.',$file,$line));
Copy link
Contributor

@srozesrozeNov 13, 2017•
edited
Loading

Choose a reason for hiding this comment

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

@Simperfit@nicolas-grekas why do we throw in here while wereturn in the other cases? This definitely smells BC break 🤔

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@nicolas-grekasnicolas-grekasnicolas-grekas left review comments

@chalasrchalasrchalasr left review comments

@stofstofstof requested changes

@fabpotfabpotfabpot approved these changes

+2 more reviewers

@TaluuTaluuTaluu left review comments

@srozesrozesroze left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

2.7

Development

Successfully merging this pull request may close these issues.

9 participants

@Simperfit@nicolas-grekas@stof@chalasr@fabpot@Taluu@sroze@xabbuh@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp