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

Run test from php 5.4 to 8.3, and fix issues with php8.2 of dependencies #512

Merged
merged 10 commits into from
Feb 17, 2024
87 changes: 82 additions & 5 deletions .github/workflows/codeigniter.yml
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,51 @@ on:
pull_request:
branches: [ master, devel ]
jobs:
required_php_versions:
name: Get PHP versions to test
runs-on: ubuntu-latest
outputs:
php_versions: ${{ steps.php_ver_step.outputs.PHP_VERSIONS }}
php_versions_matrix: ${{ steps.php_ver_step.outputs.PHP_VERSIONS_matrix }}
steps:
- name: Install required packages
run: |
if ! command -v jq; then sudo apt-get update && sudo apt-get install -y jq; fi
- name: Build PHP_VERSIONS array & PHP_VERSIONS_matrix
id: php_ver_step
run: |
set -x

# Set array that will store the PHP versions for which we create a package.
PHP_VERSIONS=()

# Get all released php versions above 5.6 (in the format X.Y)
for upstream_ver in $(curl https://www.php.net/releases/?json | jq -r '.[].version' | cut -f -2 -d .); do
major=$(cut -f 1 -d . <<< "$upstream_ver")
for minor in {0..20}; do
if dpkg --compare-versions ${major}.$minor le $upstream_ver && dpkg --compare-versions ${major}.$minor ge 5.6; then
PHP_VERSIONS+=("${major}.$minor")
fi
done
done
Comment on lines +34 to +39
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there any other simpler way to identify the versions? Maybe we could have fixed values that we support? My only concern is that is is a version released which is not supported, suddenly the CI would stop working, and throwing failure, even when other versions are supported.

Copy link
Collaborator Author

@tenzap tenzap Feb 14, 2024

Choose a reason for hiding this comment

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

Is there any other simpler way to identify the versions?

I haven't found any other way using official input.

I prefer dynamic values, that's less maintenance burden and permits to identify when there is a problem with new releases.

About failing CI we could use something like this (mark some versions as non-experimental which would make fail, the other having continue_on_failure=true) ?
https://docs.github.com/en/actions/using-jobs/using-a-matrix-for-your-jobs#handling-failures

https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#jobsjob_idcontinue-on-error

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, this seems better. Stable version shouldn't fail and would get rightly reported

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What do you mean by stable version? The list would only contain released versions of PHP, not betas or devel versions.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Correct

Copy link
Collaborator

Choose a reason for hiding this comment

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

If we have only stable versions, then all good


PHP_VERSIONS_matrix=$(sed 's/\ /", "/g' <<< [\"${PHP_VERSIONS[*]}\"])

echo "PHP_VERSIONS=${PHP_VERSIONS[*]}" >> "$GITHUB_OUTPUT"
echo "PHP_VERSIONS_matrix=$PHP_VERSIONS_matrix" >> "$GITHUB_OUTPUT"

echo "PHP_VERSIONS=${PHP_VERSIONS[*]}"
echo "PHP_VERSIONS_matrix=$PHP_VERSIONS_matrix"

test:
continue-on-error: false
needs: [ required_php_versions ]
strategy:
fail-fast: false
matrix:
operating-system: [ubuntu-latest]
php-versions: ['7.2']
#php-versions: ['5.6', '7.0', '7.1', '7.2', '7.3', '7.4', '8.0', '8.1', '8.2', '8.3']
php-versions: ${{fromJson(needs.required_php_versions.outputs.php_versions_matrix)}}
runs-on: ${{ matrix.operating-system }}
steps:
- name: Checkout
Expand Down Expand Up @@ -59,6 +99,43 @@ jobs:
args: -O gammu.sql https://raw.githubusercontent.com/gammu/gammu/master/docs/sql/mysql.sql
- name: Import Gammu DB Schema
run: mysql -h"127.0.0.1" -P"3306" -uroot -ppassword kalkun < gammu.sql
- name: Install/Update ci-phpunit-test
run: |
php vendor/kenjis/ci-phpunit-test/install.php --from-composer
# Workaround a bug in phpunit < 7 where the absence of application/tests/_ci_phpunit_test/ makes it fail
# with strpos(): Empty needle in vendor/phpunit/php-file-iterator/src/Iterator.php
if ! vendor/bin/phpunit --atleast-version 7; then
mkdir -vp application/tests/_ci_phpunit_test
# Below the alternative is to remove the culprit line from phpunit.xml
#sed -i -e "/<exclude>.\/_ci_phpunit_test\/<\/exclude>/ d" application/tests/phpunit.xml
fi

# Uncomment the monkey patcher function. This will search the line matching "Enabling Monkey Patching"
# then search the next "/*", delete that line, search the next "*/" and delete the line, write, and quit.
ed -s application/tests/Bootstrap.php <<EOF
/Enabling Monkey Patching/
/^\/\*$/
n
d
/^\*\/$/
n
d
w
q
EOF

rm application/tests/controllers/Welcome_test.php

# exlude the full application/view dir from coverage, otherwise coverage would fail.
# See: https://github.com/kenjis/ci-phpunit-test/issues/412
sed -i -e 's|<directory suffix=".php">../views/errors</directory>|<directory suffix=".php">../views</directory>|' application/tests/phpunit.xml

# the void return type of setUp() methods in phpunit (required since phpunit8) isn't supported
# with phpunit <= 6. For these, we remove the ": void" part of the tests
if [ $(composer show phpunit/phpunit | grep "^versions : " | rev | cut -d " " -f 1 | rev | cut -d . -f 1) -le 6 ]; then
sed -i "/public function setUp()/ s/ : void$//" application/tests/controllers/Install_test.php
fi

- name: Test with phpunit
run: vendor/bin/phpunit --coverage-text -c application/tests
check-code:
Expand Down Expand Up @@ -89,8 +166,8 @@ jobs:
restore-keys: ${{ runner.os }}-composer-
- name: Install dependencies
run: |
composer update
composer install --no-progress --prefer-dist --optimize-autoloader
composer update --working-dir=utils
composer install --working-dir=utils --no-progress --prefer-dist --optimize-autoloader
sudo apt-get update
# html-beautify from the debian package doesn't work for some reason
# sudo apt-get install -y node-js-beautify
Expand All @@ -101,14 +178,14 @@ jobs:
- id: check_strict_comparison
name: Check that strict comparison operators are used everywhere
run: |
git checkout composer.lock
#git checkout utils/composer.lock
git status
utils/fix_code_style.sh strict
- id: check_style
name: Check that code follows Guidelines
if: always()
run: |
git checkout composer.lock
#git checkout utils/composer.lock
git status
utils/fix_code_style.sh git-diff
- name: Archive artifacts
Expand Down
3 changes: 3 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -10,3 +10,6 @@ application/config/database.php
/dist

/utils/launchpad/dput.cf

/utils/composer.lock
/utils/vendor/
6 changes: 3 additions & 3 deletions application/tests/Bootstrap.php
Original file line number Diff line number Diff line change
Expand Up @@ -359,9 +359,9 @@
// All patchers you use.
'patcher_list' => [
'ExitPatcher',
// 'FunctionPatcher',
// 'MethodPatcher',
// 'ConstantPatcher',
'FunctionPatcher',
'MethodPatcher',
'ConstantPatcher',
],
// Additional functions to patch
'functions_to_patch' => [
Expand Down
13 changes: 12 additions & 1 deletion application/tests/controllers/Install_test.php
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,21 @@

class Install_test extends TestCase
{
private $realAssertStringContainsString;

public function setUp() : void
{
// Using assertContains() with string haystacks is deprecated and will not be supported in PHPUnit 9
// Refactor your test to use assertStringContainsString() or assertStringContainsStringIgnoringCase() instead.
$this->realAssertStringContainsString = method_exists($this, 'assertStringContainsString')
? 'assertStringContainsString'
: 'assertContains';
}

public function test_index()
{
$output = $this->request('GET', 'install');
$this->assertContains('<title>Kalkun &rsaquo; Installation</title>', $output);
call_user_func_array(array($this, $this->realAssertStringContainsString), array('<title>Kalkun &rsaquo; Installation</title>', $output));
}

public function test_method_404()
Expand Down
5 changes: 3 additions & 2 deletions application/third_party/MX/Controller.php
Original file line number Diff line number Diff line change
Expand Up @@ -39,10 +39,11 @@
class MX_Controller
{
public $autoload = array();
private $load;

public function __construct()
{
$class = str_replace(CI::$APP->config->item('controller_suffix'), '', get_class($this));
$class = str_replace(strval(CI::$APP->config->item('controller_suffix')), '', get_class($this));
log_message('debug', $class." MX_Controller Initialized");
Modules::$registry[strtolower($class)] = $this;

Expand All @@ -58,4 +59,4 @@ public function __get($class)
{
return CI::$APP->$class;
}
}
}
1 change: 1 addition & 0 deletions application/third_party/MX/Loader.php
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
class MX_Loader extends CI_Loader
{
protected $_module;
private $controller;

public $_ci_plugins = array();
public $_ci_cached_vars = array();
Expand Down
26 changes: 20 additions & 6 deletions composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -12,25 +12,39 @@
"ext-json": "*",
"ext-mbstring": "*",
"ext-session": "*",
"codeigniter/framework": ">=3.1.13 <3.2",
"codeigniter/framework": "=3.1.13",
"paragonie/random_compat": ">=2",
"giggsey/libphonenumber-for-php": "^8.12",
"league/csv": "^8.2 || ^9.5",
"datto/json-rpc-http": "^4.0 || ^5.0",
"kissifrot/php-ixr": "1.8.*",
"cweagans/composer-patches": "~1.0",
"econea/nusoap": "^0.9.5.1"
},
"require-dev": {
"phpunit/phpunit": "^7",
"kenjis/ci-phpunit-test": "^0.17.3",
"squizlabs/php_codesniffer": "^3.6",
"friendsofphp/php-cs-fixer": "^3.3",
"ise/php-codingstandards-codeigniter": "^1.0"
"phpunit/phpunit": ">=4",
"kenjis/ci-phpunit-test": ">=1"
},
"suggest": {
"ext-ldap": "For phonebook_ldap plugin",
"ext-mysqli": "To connect to a MySQL database",
"ext-pgsql": "To connect to a PostgreSQL database",
"ext-sqlite3": "To connect to a SQLite3 database"
},
"extra": {
"patches": {
"codeigniter/framework": {
"Add support for PHP 8.2 (part1)": "patches/Codeigniter_Framework/v3.1.13/10-php82_support.patch",
"Add support for PHP 8.2 (part2)": "patches/Codeigniter_Framework/v3.1.13/10-php82_support-part2.patch"
},
"kenjis/ci-phpunit-test": {
"Add support for PHP 8.2": "patches/Kenjis_CiPhpunitTest/v3.0.4/support_php-8.2.patch"
}
}
},
"config": {
"allow-plugins": {
"cweagans/composer-patches": true
}
}
}
20 changes: 20 additions & 0 deletions patches/Codeigniter_Framework/v3.1.13/10-php82_support-part2.patch
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
From 2fc2d480d0d3fc778a46ded0e093fef7a0d84257 Mon Sep 17 00:00:00 2001
From: George Petculescu <[email protected]>
Date: Fri, 3 Nov 2023 11:59:40 +0200
Subject: [PATCH] Fixes the usage of `_create_table_if` in Postgres forge class

---
system/database/drivers/postgre/postgre_forge.php | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

--- a/system/database/drivers/postgre/postgre_forge.php
+++ b/system/database/drivers/postgre/postgre_forge.php
@@ -87,7 +87,7 @@

if (version_compare($this->db->version(), '9.0', '>'))
{
- $this->create_table_if = 'CREATE TABLE IF NOT EXISTS';
+ $this->_create_table_if = 'CREATE TABLE IF NOT EXISTS';
}
}

123 changes: 123 additions & 0 deletions patches/Codeigniter_Framework/v3.1.13/10-php82_support.patch
Original file line number Diff line number Diff line change
@@ -0,0 +1,123 @@
From fb1256a5b009b6264fbc85be44e0d97654d3fcd9 Mon Sep 17 00:00:00 2001
From: George Petculescu <[email protected]>
Date: Sun, 6 Nov 2022 16:13:43 +0200
Subject: [PATCH] Adding PHP 8.2 support

---
.github/workflows/test-phpunit.yml | 20 +++++++++++++++++++-
system/core/Loader.php | 1 +
Comment on lines +1 to +8
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of having patches here, can we refer to pr's on the upstream?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

PR content might be changing in good or bad. I prefer this approach.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

BTW there is not always a PR. Here there is, but for ci-phpunit-test, there isn't.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Then maybe we should raise that to upstream?

Copy link
Collaborator Author

@tenzap tenzap Feb 15, 2024

Choose a reason for hiding this comment

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

:D it comes from upstream... bcit-ci/CodeIgniter#6173
Really I'm not in favor or using PRs, it may change and therefore it would be difficult to find a bug from someone who installs kalkun with a "PR patch", the content of which is evolving in time.
A patch in a file is the best way to stay reproducible.

Copy link
Collaborator Author

@tenzap tenzap Feb 15, 2024

Choose a reason for hiding this comment

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

oh you mean the other patch, bug is reported already. kenjis/ci-phpunit-test#413. I don't expect any change there unfortunately. Support seems to come to an end given the replies I get and the time that CI3 has been waiting for an update for recent PHP.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, okay for now, we should probably also upgrade to ci4 then given no support for ci3 anymore?

system/core/URI.php | 7 +++++++
system/database/DB_driver.php | 1 +
system/libraries/Driver.php | 1 +
system/libraries/Table.php | 4 ++--
tests/codeigniter/core/Loader_test.php | 2 +-
tests/codeigniter/libraries/Upload_test.php | 7 ++++---
tests/mocks/ci_testcase.php | 1 +
9 files changed, 37 insertions(+), 7 deletions(-)

--- a/system/core/Loader.php
+++ b/system/core/Loader.php
@@ -49,6 +49,7 @@
* @author EllisLab Dev Team
* @link https://codeigniter.com/userguide3/libraries/loader.html
*/
+#[AllowDynamicProperties]
class CI_Loader {

// All these are set automatically. Don't mess with them.
--- a/system/core/URI.php
+++ b/system/core/URI.php
@@ -52,6 +52,13 @@
class CI_URI {

/**
+ * CI_Config instance
+ *
+ * @var CI_Config
+ */
+ public $config;
+
+ /**
* List of cached URI segments
*
* @var array
--- a/system/database/DB_driver.php
+++ b/system/database/DB_driver.php
@@ -51,6 +51,7 @@
* @author EllisLab Dev Team
* @link https://codeigniter.com/userguide3/database/
*/
+#[AllowDynamicProperties]
abstract class CI_DB_driver {

/**
--- a/system/libraries/Driver.php
+++ b/system/libraries/Driver.php
@@ -50,6 +50,7 @@
* @author EllisLab Dev Team
* @link
*/
+#[AllowDynamicProperties]
class CI_Driver_Library {

/**
--- a/system/libraries/Table.php
+++ b/system/libraries/Table.php
@@ -489,12 +489,12 @@
return;
}

- $this->temp = $this->_default_template();
+ $temp = $this->_default_template();
foreach (array('table_open', 'thead_open', 'thead_close', 'heading_row_start', 'heading_row_end', 'heading_cell_start', 'heading_cell_end', 'tbody_open', 'tbody_close', 'row_start', 'row_end', 'cell_start', 'cell_end', 'row_alt_start', 'row_alt_end', 'cell_alt_start', 'cell_alt_end', 'table_close') as $val)
{
if ( ! isset($this->template[$val]))
{
- $this->template[$val] = $this->temp[$val];
+ $this->template[$val] = $temp[$val];
}
}
}
--- a/system/core/Controller.php
+++ b/system/core/Controller.php
@@ -50,6 +50,7 @@
* @author EllisLab Dev Team
* @link https://codeigniter.com/userguide3/general/controllers.html
*/
+#[AllowDynamicProperties]
class CI_Controller {

/**
--- a/system/core/Router.php
+++ b/system/core/Router.php
@@ -59,6 +59,13 @@
public $config;

/**
+ * CI_URI class object
+ *
+ * @var object
+ */
+ public $uri;
+
+ /**
* List of routes
*
* @var array
--- a/system/libraries/Image_lib.php
+++ b/system/libraries/Image_lib.php
@@ -85,6 +85,14 @@
*/
public $new_image = '';

+
+ /**
+ * Path to destination image
+ *
+ * @var string
+ */
+ public $dest_image = '';
+
/**
* Image width
*
10 changes: 10 additions & 0 deletions patches/Kenjis_CiPhpunitTest/v3.0.4/support_php-8.2.patch
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
--- a/application/tests/_ci_phpunit_test/CIPHPUnitTestCase.php
+++ b/application/tests/_ci_phpunit_test/CIPHPUnitTestCase.php
@@ -21,6 +21,7 @@
* @property CIPHPUnitTestDouble $double
* @property CIPHPUnitTestReflection $reflection
*/
+#[AllowDynamicProperties]
class CIPHPUnitTestCase extends TestCase
{
protected $_error_reporting = -1;
Loading
Loading