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

2.8 PHP7 compatability #7840

Merged
merged 44 commits into from Dec 29, 2015

Conversation

Projects
None yet
5 participants
@dereuromark
Copy link
Member

commented Dec 12, 2015

Resolves #7671 with BC.

I opened a PR to be able to work together making CakePHP 2.8+ PHP7 ready.
Anyone who can contribute, please feel free to add commits on top.
Right now it segfaults or dies away at 27%. Ideas are welcome.

dereuromark and others added some commits Nov 9, 2015

Removed last typehint Exception
#7671 Removed last typehint Exception as it was still giving an uncaught typeError message in PHP7
Merge pull request #7807 from m1nd53t/2.8-PHP7
Removed last typehint Exception

@dereuromark dereuromark added this to the 2.8.0 milestone Dec 12, 2015

@markstory

This comment has been minimized.

Copy link
Member

commented Dec 13, 2015

Have you run the suite with --debug yet? That will output the test method names as methods are run.

@phpnut

This comment has been minimized.

Copy link
Member

commented Dec 13, 2015

I think I found the issues stopping test for continuing past the 27% @dereuromark mentioned. There are a lot of places in the tests where null is being use when a class or interface is expected. Will continue later today and push changes

Larry E. Masters and others added some commits Dec 13, 2015

Larry E. Masters
session_status() not available until 5.4
Adding check for php version
Revert "Correcting return"
This reverts commit 6e09e64.
Revert "Forcing bool"
This reverts commit 26ea74c.
Revert "Forcing bool return"
This reverts commit fac95ba.
Changing return types
read now returns empty string instead of false when read method returns an empty value.
write, destroy and gc will return boolean type
@sebastienbarre

This comment has been minimized.

Copy link
Contributor

commented Dec 26, 2015

Look at the latest Travis CI build, it seems to pass the tests, or am I missing something? Just curious.

@dereuromark

This comment has been minimized.

Copy link
Member Author

commented Dec 26, 2015

@sebastienbarre No, they are failing as you can see when you look into the PHP7 one, but for some reason travis marks them as passed.

@markstory

This comment has been minimized.

Copy link
Member

commented Dec 27, 2015

Session tests are pretty sad I think @phpnut was close to having a solution on these though.

phpnut added some commits Dec 28, 2015

Fixes tests to expect changes made to read and write methods - This c…
…ould be a possible BC change

Since php 7 expects write to return true or false this needed to change, previous implementation would return the values sent to write on success and false on failure. Similar change to read method test CakeSession::read() now returns results or ''.
Reverted change setting $_SESSION to an array. Commenting out a test …
…that is invalid.

This test creates a numeric key of 0 in $_SESSION which is not a valid session key. This causes error - session_write_close(): Skipping numeric key 0 error.
Fixes SessionComponentTest::testSessionValid. Refactored _hasSession …
…and other erros on php 7

init would always set CakeSession::$_userAgent.
@phpnut

This comment has been minimized.

Copy link
Member

commented Dec 29, 2015

If this looks ok I will merge to 2.8

@@ -109,7 +109,7 @@ public function gc($expires = null) {
* @return bool True if the data was successfully cached, false on failure
*/
public function write($key, $data, $duration) {
if ($data === '' || !$this->_init) {

This comment has been minimized.

Copy link
@markstory

markstory Dec 29, 2015

Member

This will result in cache files being written for '' content. But I think that is OK and having PHP7 compatibility is more important. We might want to document this in the 2.8 migration guide though.

@@ -218,6 +217,9 @@ public static function start() {
* @return bool True if session has been started.
*/
public static function started() {
if (PHP_VERSION >= 5.4) {
return isset($_SESSION) && (session_status() === PHP_SESSION_ACTIVE);

This comment has been minimized.

Copy link
@markstory

markstory Dec 29, 2015

Member

Instead of sniffing the version, could we check for the existence of session_status()?

session_destroy();
if (isset($_COOKIE[static::_cookieName()])) {
unset($_COOKIE[static::_cookieName()]);

This comment has been minimized.

Copy link
@markstory

markstory Dec 29, 2015

Member

unset() should be safe to call on keys that are not defined.

@@ -52,7 +52,11 @@ public function close() {
* @return mixed The value of the key or false if it does not exist
*/
public function read($id) {
return Cache::read($id, Configure::read('Session.handler.config'));
$data = Cache::read($id, Configure::read('Session.handler.config'));
if (empty($data)) {

This comment has been minimized.

Copy link
@markstory

markstory Dec 29, 2015

Member

Can the data ever be '0'?

@@ -202,7 +202,8 @@ public function testSessionReadWrite() {
$this->assertEquals($Session->read('Test'), $array);
$Session->delete('Test');
$this->assertTrue($Session->write(array('Test'), 'some value'));
//This test creates a numeric key 0 which is not a valid session key
//$this->assertTrue($Session->write(array('Test'), 'some value'));

This comment has been minimized.

Copy link
@markstory

markstory Dec 29, 2015

Member

I would just remove this.

phpnut added a commit that referenced this pull request Dec 29, 2015

Merge pull request #7840 from cakephp/2.8-PHP7
2.8 PHP7 compatibility

@phpnut phpnut merged commit 0aa8847 into 2.8 Dec 29, 2015

1 of 4 checks passed

continuous-integration/travis-ci/push The Travis CI build could not complete due to an error
Details
continuous-integration/appveyor/branch AppVeyor build failed
Details
continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@phpnut phpnut deleted the 2.8-PHP7 branch Dec 29, 2015

@kaihiro kaihiro referenced this pull request Oct 18, 2017

Merged

2.8 PHP7 compatibility #8

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.