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

Fixes: a few issues found when running PHP 7.3 #12487

Merged
merged 6 commits into from Sep 9, 2018

Conversation

Projects
None yet
5 participants
@josephzidell

josephzidell commented Aug 21, 2018

I tried running 2.x on PHP 7.3, and fixed a few simple issues, such as:

  • Issue with preg_match()
  • Undefined variables
Fixes:
* Issue with preg_match()
* Undefined variables

@markstory markstory added this to the 2.10.12 milestone Aug 21, 2018

}
if ($modifiedSince) {
$timeMatches = strtotime($this->modified()) === strtotime($modifiedSince);
} else {
$timeMatches = false;

This comment has been minimized.

@markstory

markstory Aug 21, 2018

Member

You could hoist these to line 1164 and do $etagMatches = $timeMatches = false.

This comment has been minimized.

@josephzidell

This comment has been minimized.

@garas

garas Aug 21, 2018

Member

But if (empty($checks)) is now never empty

This comment has been minimized.

@josephzidell

josephzidell Aug 22, 2018

@garas How about we wrap in array_filter? if (empty(array_filter($checks)))

This comment has been minimized.

@josephzidell

This comment has been minimized.

@garas

garas Aug 22, 2018

Member

Tests fails (in 3.6 branch).

Maybe

$checks = array();
if ($responseTag) {
            $checks[] = ...;
}
if ($modifiedSince...) {
            $checks[] = ...;
}
// no compact()

There is difference [ ] vs [false, false] here.

Port these changes to 3.6 too.

This comment has been minimized.

@josephzidell

josephzidell Aug 22, 2018

Modified. I'll try to get to 3.6 later

joezidell-saleswarp added some commits Aug 21, 2018

@garas

Tests fail

@@ -95,6 +95,7 @@ public function trigger($callback, $params = array(), $options = array()) {
if (empty($this->_enabled)) {
return true;
}
$subject = null;

This comment has been minimized.

@garas

garas Aug 22, 2018

Member

Tests fail here too.

Very similar problem.


To CakePHP Team: Why Travis tests are not running on 2.x?

This comment has been minimized.

@markstory

markstory Aug 22, 2018

Member

To CakePHP Team: Why Travis tests are not running on 2.x?

I'm not sure. Travis used to run tests for 2.x Its possible that there is a problem with the configuration file, or perhaps we're trying to use an unsupported worker type.

This comment has been minimized.

@markstory

markstory Aug 22, 2018

Member

It looks like travis ran tests for 2.x 8 days ago I'm not sure why it isn't running this pull request.

This comment has been minimized.

@josephzidell

josephzidell Aug 27, 2018

Can we merge this as this seems to be a Travis issue?

This comment has been minimized.

@garas

garas Aug 27, 2018

Member

Tests fail here, so should be fixed

This comment has been minimized.

@garas

garas Aug 29, 2018

Member

Run tests:

lib/Cake/Console/cake test core AllTests

You can also run only some test cases like Utility/ObjectCollection or Network/CakeResponse

This comment has been minimized.

@josephzidell

josephzidell Aug 29, 2018

I cannot install PHPUnit 3.7, as I already have 7.3 installed on my machine. Can you help me here, by showing me the issue? Could not find it in the PR tools

This comment has been minimized.

@garas

garas Aug 29, 2018

Member

You can install phpunit in CakePHP dir with composer require --dev phpunit/phpunit:"3.7.38"

This comment has been minimized.

@josephzidell

josephzidell Sep 3, 2018

Pushed a fix, and now both tests are passing

This comment has been minimized.

@josephzidell

josephzidell Sep 7, 2018

Can this get merged?

@@ -95,6 +95,7 @@ public function trigger($callback, $params = array(), $options = array()) {
if (empty($this->_enabled)) {
return true;
}
$subject = null;

This comment has been minimized.

@garas

garas Aug 27, 2018

Member

Tests fail here, so should be fixed

@markstory markstory modified the milestones: 2.10.12, 2.10.13 Sep 3, 2018

@markstory

This comment has been minimized.

Member

markstory commented Sep 8, 2018

@josephzidell I will get this merged when I get a chance to run tests locally, as travis is not cooperating.

@markstory markstory self-assigned this Sep 8, 2018

@josephzidell

This comment has been minimized.

josephzidell commented Sep 8, 2018

Thanks @markstory

@markstory markstory merged commit 2bea29d into cakephp:2.x Sep 9, 2018

1 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
stickler-ci Pull request targets an ignored branch
@josephzidell

This comment has been minimized.

josephzidell commented Sep 9, 2018

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