2
\$\begingroup\$

I have inherited a project in bad conditions that contains the following class and I need your help to improve it, the truth is I don't know if it is correctly optimized or if it has security problems or if it can be simplified or refactored to mitigate some overload / overheating . I can't afford to scrap the entire project because of this class.

php v7.4

I proceed to explain what the class does, This class has 3 main objectives related to its sole responsibility:

responsibility: it is a bridge to the implementation of procedural and simple classes.

To achieve responsibility, 3 objectives are implemented:

  1. Provide Access to prosedural functions by associating to a Generic class.
  2. Extract from a directory the php files that correspond to the procedural libraries (where are the functions).
  3. implement the magic method __call and call_user_func_array to facilitate the above common goals

Functional code example:

index.php

<?php
require_once 'function_wrapper.php';

class Main
{

    public static $funcObj = null;

    public function run()
    {

        $wrapper = new functionWrapper();
        $wrapper->proceduralFunctionLoader();

        echo '<pre>';
        echo var_dump(self::$funcObj['TEST']);
        echo '</pre>';
        echo '<br>';
        self::$funcObj['TEST']->testFoo();


    }

}


$main = new Main();
$main->run();

function_wrapper.php

<?php

class functionWrapper
{

    public function proceduralFunctionLoader()
    {
        $libpath      = $_SERVER["DOCUMENT_ROOT"].'lib/';
        $files        = array_diff(scandir($libpath, 1), ['..', '.']);
        $functionList = [];

        if (!empty($files)) {
            Main::$funcObj['TEST'] = [];
            foreach ($files as $key => $name) {
                if (strpos($name, 'lib.') !== false) {
                    $classDir = $libpath.$name;
                    require_once $classDir;
                    $functionList[$name] = $this->getFunctionsInFile($classDir);
                }
            }
            Main::$funcObj['TEST'] = $this;
        }
    }

    public function getFunctionsInFile($classDir)
    {
        $source           = file_get_contents($classDir);
        $tokens           = token_get_all($source);
        $functions        = [];
        $nextStringIsFunc = false;
        $inClass          = false;
        $bracesCount      = 0;
        foreach ($tokens as $token) {
            switch ($token[0]) {
                case T_CLASS:
                    $inClass = true;
                    break;
                case T_FUNCTION:
                    if (!$inClass) {
                        $nextStringIsFunc = true;
                    }
                    break;
                case T_STRING:
                    if ($nextStringIsFunc) {
                        $nextStringIsFunc = false;
                        $functions[]      = $token[1];
                    }
                    break;
                case '(':
                case ';':
                    $nextStringIsFunc = false;
                    break;
                case '{':
                    if ($inClass) {
                        ++$bracesCount;
                    }
                    break;
                case '}':
                    if ($inClass) {
                        --$bracesCount;
                        if (0 === $bracesCount) {
                            $inClass = false;
                        }
                    }
                    break;
            }
        }

        return $functions;
    }

    public function __call($functionName, $args)
    {
        try {
            $result = null;
            if (function_exists($functionName)) {
                $result = call_user_func_array($functionName, $args);
            }
            if (null === $result) {
                throw new Exception('A Function that does not exist has been Invoked in the System: '.$functionName.'. <br>');
            }

            return $result;
        } catch (Exception $e) {
            echo '<pre>';
            echo var_dump($e);
            echo '</pre>';
            exit();
        }
    }
}

/lib directory have 2 files inside:

lib.test.php

<?php
function testFoo2()
{
    echo '<h1>Hello World!!!</h1>';
}

lib.test2.php

<?php
function testFoo()
{
    Main::$funcObj['TEST']->testFoo2();

    return true;
}

I find one additional problems:

procedural script is forced to return true or any data; if it return nothing or false; trow error like call_user_func_array error documentation.

if i try implement a validator (expected to return true or false); how can i differentiate if the false return is equivalent to an error or the validation (function) returns false?

\$\endgroup\$
5
  • \$\begingroup\$ To me this code makes no sense. Why not simply include the files that contain the classes/functions? That will be far more efficient and is the "normal" way of doing it. I'm sure there's some hidden reason. Perhaps you don't want the classes/functions in the global scope? To do that PHP has namespaces. So, I don't get it. Also, the use of the $funcObj static variable is weird, but I guess it's only there for testing purposes? If it is, it should have been removed for this question. \$\endgroup\$ May 24, 2022 at 10:43
  • \$\begingroup\$ I'm not looking to remove what already exists here, I'm looking to improve it... if you ask me personally... I would also have removed it and implemented composer autoloader etc... \$\endgroup\$
    – user152284
    May 24, 2022 at 14:22
  • \$\begingroup\$ the examples I've put are only about 4% of the code overall; If I eliminate the use of the static variable and I eliminate the dynamic require_once... I would be breaking the entire project that they have passed me, it would cost more to make the manual changes since phpstorm for example would not refactor the magic methods that are implemented with the use of variables static. \$\endgroup\$
    – user152284
    May 24, 2022 at 14:28
  • \$\begingroup\$ Yes, I know you must have your reasons for this code, but it is hard to "improve" code when you have all these hidden restrictions on changes. As far as I can tell this code doesn't suffer from any security problem: It basically parses all the PHP files, every time the script is executed, to find classes/functions and allow you to call those. It does what PHP normally does automatically. It's pointless, but not a security risk. You could speed the code up a lot by caching the results of the parsing, but you didn't state that slowness was a problem. \$\endgroup\$ May 24, 2022 at 14:42
  • \$\begingroup\$ @KIKOSoftware gracias, I'll be giving a couple more laps to the last one, and the validation of the returned data. \$\endgroup\$
    – user152284
    May 24, 2022 at 14:48

0

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.