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.x Build and Pass with PHP7.2 #11619

Merged
merged 11 commits into from Jan 19, 2018
Merged

2.x Build and Pass with PHP7.2 #11619

merged 11 commits into from Jan 19, 2018

Conversation

tenkoma
Copy link
Member

@tenkoma tenkoma commented Jan 10, 2018

Refs #11346

In this Pull Request, Installed the mcrypt extension from pecl in the CI so that we can test it.

  • added pear config-set preferred_state snapshot. Because pecl's mcrypt package is snapshot, is not stable https://pecl.php.net/package/mcrypt
  • I changed pecl install to execute first. Because fails composer require "phpunit/phpunit=$PHPUNIT"
  • remove cannot run test matrix: PHPUnit 3.7 + PHP7.2

@dereuromark
Copy link
Member

Now you are getting somewhere, errors become visible:

  • count(): Parameter must be an array or an object that implements Countable
  • ini_set(): A session is active. You cannot change the session module's ini settings at this time

Seem to be the issues left for PHP7.2 compatibility.

@dereuromark dereuromark added this to the 2.10.7 milestone Jan 10, 2018
@markstory
Copy link
Member

ini_set(): A session is active. You cannot change the session module's ini settings at this time

These are a big headache to fix.

@tenkoma
Copy link
Member Author

tenkoma commented Jan 15, 2018

ini_set(): A session is active. You cannot change the session module's ini settings at this time

I didn't fail when I only ran CakeSessionTest.
There was a problem with other tests.
But a new error was found.

ini_set(): Cannot set 'user' save handler by ini_set() or session_module_name()

Probably related to the next ticket.

https://bugs.php.net/bug.php?id=73100

@tenkoma tenkoma changed the title 2.x Build with PHP7.2 (Test still fails) WIP - 2.x Build with PHP7.2 (Test still fails) Jan 15, 2018
// https://github.com/php/php-src/commit/a93a51c3bf4ea1638ce0adc4a899cb93531b9f0d
if (version_compare(PHP_VERSION, '7.2.0', '>=')) {
unset($sessionConfig['ini']['session.save_handler']);
}
Copy link
Member Author

@tenkoma tenkoma Jan 17, 2018

Choose a reason for hiding this comment

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

I referred to the following code. However, I think it is PHP 7.2.0+ that this setting cannot be done

// In PHP7.1.0+ session.save_handler can't be set to user by the user.
// https://github.com/php/php-src/blob/master/ext/session/session.c#L559
if (!empty($sessionConfig['handler']) && version_compare(PHP_VERSION, '7.1.0', '<=')) {
$sessionConfig['ini']['session.save_handler'] = 'user';

Copy link
Member

Choose a reason for hiding this comment

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

I think you have the correct code here. There is an open issue for database sessions in PHP7.2 (#11628)

@tenkoma tenkoma changed the title WIP - 2.x Build with PHP7.2 (Test still fails) 2.x Build and Pass with PHP7.2 Jan 17, 2018
@@ -328,7 +328,7 @@ public function bakeActions($controllerName, $admin = null, $wannaUseSession = t
* @param array $components Components to use in controller
* @return string Baked controller
*/
public function bake($controllerName, $actions = '', $helpers = null, $components = null) {
public function bake($controllerName, $actions = '', $helpers = array(), $components = array()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you change this signature?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is a change to correct errors that occur in the following lines.

https://github.com/cakephp/cakephp/blob/2.x/lib/Cake/Console/Templates/default/classes/controller.ctp#L51

But it is not BC with existing applications.
Make null be converted to array().

$this->out("\n" . __d('cake_console', 'Baking controller class for %s...', $controllerName), 1, Shell::QUIET);

if (is_null($helpers)) {
Copy link
Member

Choose a reason for hiding this comment

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

We usually favor $helpers === null as it uses fewer function calls.

@markstory markstory self-assigned this Jan 19, 2018
@markstory markstory merged commit 1e87526 into cakephp:2.x Jan 19, 2018
markstory added a commit that referenced this pull request Jan 19, 2018
Forward port changes from #11619 and fix #11628
@tenkoma tenkoma deleted the 2.x-build-php72 branch January 20, 2018 02:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants