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

Test also on PHP 7.2+ #3186

Merged
merged 1 commit into from Jun 8, 2018

Conversation

Projects
None yet
6 participants
@Al2Klimov
Copy link
Contributor

Al2Klimov commented Dec 4, 2017

No description provided.

@Al2Klimov Al2Klimov requested a review from lippserd Dec 4, 2017

@Al2Klimov

This comment has been minimized.

Copy link
Contributor Author

Al2Klimov commented Dec 4, 2017

Rationale: Who knows all the upcoming changes to PHP and our obstacles?

@dnsmichi

This comment has been minimized.

Copy link
Member

dnsmichi commented Dec 4, 2017

Mh if nightlies contain bugs, this will break travis tests on PRs. The benefit of testing the git master against nightlies is then gone. Imho this should be done in a Jenkins job, separate from code commits.

@Al2Klimov

This comment has been minimized.

Copy link
Contributor Author

Al2Klimov commented Dec 4, 2017

@dnsmichi Well, if even this PR fails to be tested... then you're damn right.

@Thomas-Gelf

This comment has been minimized.

Copy link
Contributor

Thomas-Gelf commented Dec 4, 2017

Hint: add allow_failures -> - php: nightly to your matrix

@Al2Klimov Al2Klimov force-pushed the feature/test-php-nightly branch from ec8ed6b to 2459f34 Dec 4, 2017

@Al2Klimov Al2Klimov removed the request for review from lippserd Dec 5, 2017

@@ -531,7 +531,7 @@ protected function loadConfig()
*/
protected function setupErrorHandling()
{
error_reporting(E_ALL | E_STRICT);
error_reporting((E_ALL | E_STRICT) & ~ E_DEPRECATED);

This comment has been minimized.

@dnsmichi

dnsmichi Dec 5, 2017

Member

Mh you should see them imho. This allows to react sooner on possibly removed things, especially if you are really testing the new "hot shit".

This comment has been minimized.

@Al2Klimov

Al2Klimov Dec 5, 2017

Author Contributor

Just testing. Don't panic.

This comment has been minimized.

@dnsmichi

dnsmichi Dec 5, 2017

Member

Then edit the PR's title to include "WIP: " as prefix :)

@Al2Klimov Al2Klimov force-pushed the feature/test-php-nightly branch 3 times, most recently from dcba443 to 4dc6b5e Dec 5, 2017

@Al2Klimov

This comment has been minimized.

Copy link
Contributor Author

Al2Klimov commented Dec 5, 2017

It seems PHP 7.2 has broken our session reopening introduced in efa23ad.

@Al2Klimov

This comment has been minimized.

Copy link
Contributor Author

Al2Klimov commented Dec 5, 2017

  1. The tests fail on PHP 7.2+ because of our session reopening.
  2. Our session reopening is required as we use session_write_close().

But why don't we close the session just once – at shutdown?

CC @lippserd @Thomas-Gelf @nilmerg

@lippserd

This comment has been minimized.

Copy link
Member

lippserd commented Dec 5, 2017

@Al2Klimov session_start() locks the session file until session_write_close() is called. We close the session immediately after read/write operations to prevent Ajax requests from blocking. Smart eh? 😆

@Al2Klimov Al2Klimov force-pushed the feature/test-php-nightly branch from a345e2f to 9585f47 Dec 7, 2017

@Al2Klimov Al2Klimov requested review from Thomas-Gelf , lippserd and nilmerg Dec 7, 2017

@Al2Klimov

This comment has been minimized.

Copy link
Contributor Author

Al2Klimov commented Dec 7, 2017

Gentlemen, it's your turn. B-)

@Al2Klimov Al2Klimov changed the title Test also on PHP nightly Test also on PHP 7.2+ Dec 7, 2017

@Al2Klimov Al2Klimov referenced this pull request Dec 8, 2017

Closed

PHP 7.2 Support? #3185

@Thomas-Gelf

This comment has been minimized.

Copy link
Contributor

Thomas-Gelf commented Dec 8, 2017

Short note: the former session code looks like it tried to avoid issues with "headers already sent". No idea for what use case, but it looks like the new variant would break that countermeasure. Eventually @lippserd can have a look at this. Filter -> good catch! Static code analysis is not that bad at all 😆.

@Al2Klimov

This comment has been minimized.

Copy link
Contributor Author

Al2Klimov commented Dec 8, 2017

@Thomas-Gelf What break? I've tested that stuff w/

class IndexController extends Controller
{
    protected $requiresAuthentication = false;

    public function indexAction()
    {
        $s = Session::getSession();
        $s->write();
        $s->write();
        $s->write();
        var_dump(headers_list());
    }
}

The former session code (efa23ad) was requested by you to "Allow sessions to be reopened" – and it still works.

@Thomas-Gelf

This comment has been minimized.

Copy link
Contributor

Thomas-Gelf commented Dec 8, 2017

Nothing is wrong with your code alone, and I'm sure tests are fine. So, it's no break per se, but it changes current behavior. To me the old code looks like it has intentionally been placed there. It told PHP to not send headers on it's own (triggered by session_start) at that early stage. There might be a reason for this, other components might rely on deferred header sending. But you should better discuss this with @lippserd.

@Al2Klimov

This comment has been minimized.

Copy link
Contributor Author

Al2Klimov commented Dec 8, 2017

... or w/ @nilmerg the author of efa23ad.

@Thomas-Gelf

This comment has been minimized.

Copy link
Contributor

Thomas-Gelf commented Dec 8, 2017

@Al2Klimov: just in case you're wondering, the referenced issue 5510 refers to dev.icinga.org, it's #457 here on GitHub.

@Al2Klimov

This comment has been minimized.

Copy link
Contributor Author

Al2Klimov commented Dec 8, 2017

No, I'm not. But thank you anyway.

@Al2Klimov

This comment has been minimized.

Copy link
Contributor Author

Al2Klimov commented Dec 8, 2017

Just tested w/

class IndexController extends Controller
{
    protected $requiresAuthentication = false;

    public function indexAction()
    {
        $s = Session::getSession();
        $s->write();
        while (ob_get_level()) {
            ob_end_clean();
        }
        echo 'foobar';
        $s->write();
        var_dump(headers_list());
    }
}

(It doesn't work.)

@Al2Klimov

This comment has been minimized.

Copy link
Contributor Author

Al2Klimov commented Dec 8, 2017

... but IMO it doesn't break anything... except write operations at shutdown time. :/

@Al2Klimov

This comment has been minimized.

Copy link
Contributor Author

Al2Klimov commented Jan 24, 2018

Why "a dedicated one" – and not this one? One nice day we just turn it on (disallow failing).

@Al2Klimov

This comment has been minimized.

Copy link
Contributor Author

Al2Klimov commented Jan 24, 2018

@Al2Klimov Al2Klimov force-pushed the feature/test-php-nightly branch from 6e4f86b to f334a9e Jan 24, 2018

@Al2Klimov

This comment has been minimized.

Copy link
Contributor Author

Al2Klimov commented Jan 24, 2018

@Thomas-Gelf We don't have to allow Travis to fail – if you give green light on #3213

@Thomas-Gelf

This comment has been minimized.

Copy link
Contributor

Thomas-Gelf commented Jan 24, 2018

Perfect, merged all three of them, thank you. Same game again please for f334a9e and cf85e66. Also for f8a71b7, but there please extend the commit message and explain your motivation for this commit. Is our default error_reporting too low? Or too strict on PHP 7.2? What would fail (or not fail) without this change?

@Thomas-Gelf Thomas-Gelf reopened this Jan 24, 2018

@Al2Klimov

This comment has been minimized.

Copy link
Contributor Author

Al2Klimov commented Jan 24, 2018

cf85e66? IMO we need this only here.

@Al2Klimov Al2Klimov force-pushed the feature/test-php-nightly branch from f334a9e to 4dd90cd Jan 24, 2018

@Al2Klimov

This comment has been minimized.

Copy link
Contributor Author

Al2Klimov commented Jan 24, 2018

@Thomas-Gelf

This comment has been minimized.

Copy link
Contributor

Thomas-Gelf commented Jan 24, 2018

The only commit that will remain in this pull request is 2d5140a - as it is the only one that fit's the topic. The others fix related issues. To me cf85e66 is clearly a bugfix. Parts of this have been triggered by 91b0e98, one line is older. I'm wondering why this didn't fail earlier, I would have expected also older PHP versions to warn on this.

Next, what was the motivation for 6fc90be (PhpCodeValidityTest)? It does obviously more than php -l, as it also loads the code. Also, lint-alike errors would usually be caught by phpcs.

And the last big beast is the session. That one requires special care. Some weird things in the original implementation derive from locking/performance reasons, as @lippserd pointed out. Not writing when nothing changed always makes sense, it saves a lot of I/O. Still, even with no change from time to time we need to force a write, as otherwise GC would kill our session.

Immediately closing the session on write is even more critical, as the default PHP behavior would have devastating effect on our performance. And what remains last: modules like Nagvis are forced to do weird things with sessions. They shut one session down, start the other one, switch back - none of this works reliably when PHP headers are sent out early. This and related issues are what those INI settings should have taken care of.

To keep things short: there is too much duplicated code in PhpSession72. Please have a look at the write() method, just to name one example. There is one differing line, no need to clone the whole method. Please isolate the tasks causing trouble on 7.2 in the original file into dedicate methods and then override only them.

Thanks,
Thomas

@Al2Klimov Al2Klimov force-pushed the feature/test-php-nightly branch from 4dd90cd to c8930f3 Jan 24, 2018

@Al2Klimov

This comment has been minimized.

Copy link
Contributor Author

Al2Klimov commented Jan 24, 2018

Now as I've outsourced #3310, this works w/o #3213.

@Thomas-Gelf

This comment has been minimized.

Copy link
Contributor

Thomas-Gelf commented Jan 24, 2018

We're getting closer :-)

@Thomas-Gelf

This comment has been minimized.

Copy link
Contributor

Thomas-Gelf commented Jan 24, 2018

I cherry-picked c8930f3 to speed things up. Please move the fixes for the Session to a dedicated pull request and address above change requests. Thanks for the good work!

@Al2Klimov Al2Klimov force-pushed the feature/test-php-nightly branch 2 times, most recently from 055dca6 to 45a508e Jan 24, 2018

@Al2Klimov Al2Klimov requested a review from Thomas-Gelf Jan 24, 2018

@Al2Klimov Al2Klimov force-pushed the feature/test-php-nightly branch from 45a508e to 920aa4a Jan 24, 2018

@Al2Klimov

This comment has been minimized.

Copy link
Contributor Author

Al2Klimov commented Jan 24, 2018

@Thomas-Gelf As long as Travis fails, please wait until I've rebased the other merged PRs.

@SMillerDev

This comment has been minimized.

Copy link

SMillerDev commented Feb 22, 2018

This is only waiting for the pr to drop 5.3 then?

@Al2Klimov

This comment has been minimized.

Copy link
Contributor Author

Al2Klimov commented Feb 26, 2018

@SMillerDev No, it isn't.

@lippserd lippserd removed the low-priority label May 7, 2018

@lippserd lippserd force-pushed the feature/test-php-nightly branch from 920aa4a to a77ea2f Jun 8, 2018

@lippserd lippserd removed the needs-feedback label Jun 8, 2018

@lippserd lippserd added this to the 2.6.0 milestone Jun 8, 2018

@lippserd lippserd merged commit 6ab87cf into master Jun 8, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@lippserd lippserd deleted the feature/test-php-nightly branch Jun 8, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.