Move C3 code to separate class (2023) #85

marcovtwout · 2023-04-04T12:27:58Z

This PR is a first step in improving #40 applying the same solution as #72
Instead of having a copy of c3.php in the project root it can be included from any location.

Basic usage:

require __DIR__ . '/vendor/autoload.php'; // If using Composer
//require __DIR__ . '/vendor/codeception/c3/c3.php'; // Or include it directly

$c3 = new Codeception\C3(__DIR__); // pass the directory that contains Codeception config files
$c3->run();

I only made the minimal changes to use this concept. Best to disable whitespace comparison when reviewing this PR.

There's more to be improved after this:

Since this probably means a new major version of c3, we could remove legacy options for outdated autoloading, codeception and phpunit versions
Add options to configure error log file and temp output directory
Make all __c3_-functions part of the class
Restructure remaining code blocks into functions

But please review this PR first and decide if this could start a new v3 branch. If so, I'd be happy to submit further improvement PR's on top of that (based on main...marcovtwout:c3:class_based_c3_improvements).

Basic usage: ``` require __DIR__ . '/vendor/codeception/c3/c3.php'; $c3 = new Codeception\C3(__DIR__); $c3->run(); ```

marcovtwout · 2023-05-03T14:03:00Z

@Naktibalda Do you need any help getting this reviewed? I think it would be really benificial for the maintainability of this project if it becomes a composer requirement through this PR.

marcovtwout · 2023-08-21T07:41:32Z

@TavoNiievez Is there anyone else that can review this?

delboy1978uk · 2023-10-27T09:52:59Z

@marcovtwout looks good (haven't tried it), you should resolve conflicts and update this line for the tests here with the latest PHP versions, and drop the ones that are no longer supported https://github.com/Codeception/c3/blob/main/.github/workflows/main.yml#L11
Official support for PHP 8.0 ends in January https://www.php.net/supported-versions.php

marcovtwout · 2023-12-18T16:30:07Z

@delboy1978uk That line is not part of this PR, but I can rebase and fix conflicts again and again.. but there is no point until somebody reviews this first.

So can somebody please review this? :)

samdark · 2023-12-20T11:53:23Z

c3.php


-if (isset($_COOKIE['CODECEPTION_CODECOVERAGE'])) {
-    $cookie = json_decode($_COOKIE['CODECEPTION_CODECOVERAGE'], true);
+// $_SERVER['HTTP_X_CODECEPTION_CODECOVERAGE_DEBUG'] = 1;


I guess this one was there as a configuration option. How to configure it now if the file isn't yours?

This (commented) code was only moved from above the use-statements to below. Functionality is unchanged.

SamMousa

I've given a lot of detailed comments and they could lead to the following conclusion:

This new release will be a backwards compatibility break
We could use this to drop support for older versions of Codeception / PHPUnit

Most if this code is very old and therefore very messy. It could be immensely simplified:

Remove constants, use constructor arguments for configuration
Remove workarounds for class loading, it's 2023, we use composer autoloader, with additional support for Codeception PHAR.

Additionally, I'm not sure why we need to load codeception configuration at all, or need codeception classes for that matter. Couldn't we just rewrite this as a thin wrapper on the code coverage package which has been improved since C3 was originally written?

SamMousa · 2023-12-20T12:08:22Z

c3.php

+    public function setAutoloadRootDir($dir)
+    {
+        $this->autoloadRootDir = $dir;


I don't think the autoloadRootDir should be mutable. I'm not even sure it's needed at all in this class.

SamMousa · 2023-12-20T12:09:43Z

c3.php

-        ini_set('memory_limit', $requiredMemory);
-    }
+        // Autoload Codeception classes
+        if (!class_exists('\\Codeception\\Codecept') || !function_exists('codecept_is_path_absolute')) {


Use ::class (and add import at top)

Suggested change

if (!class_exists('\\Codeception\\Codecept') || !function_exists('codecept_is_path_absolute')) {

if (!class_exists(Codecept::class) || !function_exists('codecept_is_path_absolute')) {

SamMousa · 2023-12-20T12:12:07Z

c3.php

+            } elseif (stream_resolve_include_path($this->autoloadRootDir . '/vendor/autoload.php')) {
+                require_once $this->autoloadRootDir . '/vendor/autoload.php';


We should never include the composer autoloader, since it's responsible for loading our class. So I'd say remvoe this code path.

The only thing we should manually autoload are:

Codeception from phar file

SamMousa · 2023-12-20T12:12:29Z

c3.php

+                if (stream_resolve_include_path($this->autoloadRootDir . '/vendor/codeception/codeception/autoload.php')) {
+                    require_once $this->autoloadRootDir . '/vendor/codeception/codeception/autoload.php';
+                }
+            } elseif (stream_resolve_include_path('Codeception/autoload.php')) {
+                require_once 'Codeception/autoload.php';


From inspecting that file I don't think we actually need to include it: https://github.com/Codeception/Codeception/blob/5.0/autoload.php

SamMousa · 2023-12-20T12:14:16Z

c3.php

-    define('C3_CODECOVERAGE_PROJECT_ROOT', Codeception\Configuration::projectDir());
-    define('C3_CODECOVERAGE_TESTNAME', $_SERVER['HTTP_X_CODECEPTION_CODECOVERAGE']);
+        // phpunit codecoverage shimming
+        if (!class_exists('PHP_CodeCoverage') and class_exists('SebastianBergmann\CodeCoverage\CodeCoverage')) {


This class no longer exist (it was removed ~2012) from PHPUnit. I propose not supporting it in this new version of C3. Just remove this shimming.

SamMousa · 2023-12-20T12:17:03Z

c3.php

+        if (isset($_SERVER['HTTP_X_CODECEPTION_CODECOVERAGE_CONFIG'])) {
+            $configFile = $this->configDir . DIRECTORY_SEPARATOR . $_SERVER['HTTP_X_CODECEPTION_CODECOVERAGE_CONFIG'];
+        }


I'm personally not a fan of having this artifact in the code. We should not be dynamically loading user supplied paths.
I imagine this is a very niche cache where you want to dynamically configure it for C3 based on the request.

SamMousa · 2023-12-20T12:18:32Z

c3.php

-$requestedC3Report = (strpos($_SERVER['REQUEST_URI'], 'c3/report') !== false);
+            function __c3_clear()
+            {
+                \Codeception\Util\FileSystem::doEmptyDir(C3_CODECOVERAGE_MEDIATE_STORAGE);


We should not be using this constant.
Instead:

define it as a constructor argument

store it in a private property

in the constructor check if the constant is defined and if so use it to initialize the private property

remove all references to this constant

SamMousa · 2023-12-20T12:19:39Z

c3.php

+            if ($cookie) {
+                foreach ($cookie as $key => $value) {
+                    if (!empty($value)) {
+                        $_SERVER["HTTP_X_CODECEPTION_" . strtoupper($key)] = $value;


Don't use the global $_SERVER instead add a constructor param $server.
This makes the dependencies clear from the call signature:

(new C3($_SERVER, $_REQUEST, $argument1, ...))->run();

marcovtwout · 2023-12-21T11:50:27Z

@SamMousa Thanks for your review!

I agree exactly with your conclusions, but I have split them up into two parts for the sake of easier reviewing and continued development:

Step 1, create a new major version based on this PR so we can use it as a Composer dependency. The result could already be used by anyone with existing setups, with only a minimal change in your projects and low risk of breaking things.
Step 2, start development on the next major version to get rid of many legacy options and improve code quality. For a great part my suggestions overlap with yours. See the commits I created on this derived branch: main...marcovtwout:c3:class_based_c3_improvements

SamMousa · 2023-12-21T12:49:23Z

Cool, go ahead with step 1 then. For the new implementation I'd say have a look at #93! If you want to give hte implementation a try go ahead.

marcovtwout added 3 commits April 4, 2023 14:04

Move all c3 code to a separate class Codeception\C3.

c5947ff

Basic usage: ``` require __DIR__ . '/vendor/codeception/c3/c3.php'; $c3 = new Codeception\C3(__DIR__); $c3->run(); ```

Remove installer and configure composer autoloader

f0662c0

Update installation and usage instructions, anticipating version bumps

c4a7a36

marcovtwout mentioned this pull request Apr 4, 2023

Move C3 code to separate class #72

Closed

TavoNiievez requested a review from Naktibalda April 15, 2023 00:13

samdark approved these changes Dec 20, 2023

View reviewed changes

samdark requested a review from SamMousa December 20, 2023 11:54

SamMousa reviewed Dec 20, 2023

View reviewed changes

SamMousa mentioned this pull request Dec 20, 2023

New idea for remote CC #93

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move C3 code to separate class (2023) #85

Move C3 code to separate class (2023) #85

marcovtwout commented Apr 4, 2023

marcovtwout commented May 3, 2023

marcovtwout commented Aug 21, 2023

delboy1978uk commented Oct 27, 2023

marcovtwout commented Dec 18, 2023

samdark Dec 20, 2023

marcovtwout Dec 21, 2023

SamMousa left a comment

SamMousa Dec 20, 2023

SamMousa Dec 20, 2023

SamMousa Dec 20, 2023

SamMousa Dec 20, 2023

SamMousa Dec 20, 2023

SamMousa Dec 20, 2023

SamMousa Dec 20, 2023

SamMousa Dec 20, 2023

marcovtwout commented Dec 21, 2023

SamMousa commented Dec 21, 2023

	if (!class_exists('\\Codeception\\Codecept') \|\| !function_exists('codecept_is_path_absolute')) {
	if (!class_exists(Codecept::class) \|\| !function_exists('codecept_is_path_absolute')) {

		} elseif (stream_resolve_include_path($this->autoloadRootDir . '/vendor/autoload.php')) {
		require_once $this->autoloadRootDir . '/vendor/autoload.php';

Move C3 code to separate class (2023) #85

Are you sure you want to change the base?

Move C3 code to separate class (2023) #85

Conversation

marcovtwout commented Apr 4, 2023

marcovtwout commented May 3, 2023

marcovtwout commented Aug 21, 2023

delboy1978uk commented Oct 27, 2023

marcovtwout commented Dec 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SamMousa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcovtwout commented Dec 21, 2023

SamMousa commented Dec 21, 2023