Documentation issue and Infinite loop instantiating in non CLI mode

[chrisdeeming]

First, documentation.

The README.md file contains a section about instantiating and using this programmatically:

use AMWScan\Scanner;

$app = new Scanner();
$report = $app->setPathScan("my/path/to/scan")
              ->enableBackups()
              ->setPathBackups("/my/path/backups")
              ->enableLiteMode()
              ->setAutoClean()
              ->run();

However this example is invalid due to the fact that the majority of those class methods are actually static.

It has to be said that the documented approach would definitely be preferable as it is a more common object oriented approach.

All or most of those method calls above actually instantiate the object again from scratch because they tend to end with:

return new self();

I guess the documentation needs to be something like this but you'd be instantiating the class at least 6 times:

use AMWScan\Scanner;

Scanner::setPathScan("my/path/to/scan");
Scanner::enableBackups();
Scanner::setPathBackups("/my/path/backups");
Scanner::enableLiteMode();
Scanner::setAutoClean();

$app = new Scanner();

However, the bigger problem with this approach is the __construct method if you're not running via the CLI:

if (!self::isCli()) {
    self::setSilentMode();
}

In case the issue isn't obvious, the setSilentMode method ends in:

return new self();

So you have a code path in the __construct method which results in calling the __construct method therefore there is an infinite loop.

My gut feeling is that the best approach would be to move away from these static methods entirely and make them public class methods and use class properties:

    public function setPathScan($pathScan)
    {
        $this->pathScan = Path::get($pathScan);

        return $this;
    }

This would result in the documented example actually working - assuming similar changes are applied to all of the methods, though there is at least 52 methods that would need to be changed and I suspect there would be many more changes to accommodate that.

$scanner = new Scanner();
$scanner->setPathScan("path")
    ->enableBackups()
    ->setAutoClean();

The simplest solution may be to update the documentation and also change these methods to no longer return themselves. I don't tend to see a lot of that with static method calls so it strikes me as strange but I'm saying that with the ignorance of only having seen a little bit of the Scanner class and not fully understanding the full library.

Any thoughts?

[marcocesarato]

Hi @chrisdeeming, this library has been rewritten many times so some logic comes from previous versions (initially it was a single script file just to give an idea) and programmatic usage was not maintained with the same priority as using the CLI.

Anyway, originally the idea was to use it like a singleton instance and if you check the line below, it theoretically prevents the infinite loop you intend using fluent setters with return new self();.

PHP-Antimalware-Scanner/src/Scanner.php

Line 301 in f17ae16

if (!self::isInitialized()) {

PHP-Antimalware-Scanner/src/Scanner.php

Line 325 in f17ae16

self::$inited = true;

The below pattern usage is permitted from php.

$report = $app->setPathScan("my/path/to/scan")
              ->enableBackups()
              ->setPathBackups("/my/path/backups")
              ->enableLiteMode()
              ->setAutoClean()
              ->run();

and also the following

Scanner::setPathScan("my/path/to/scan");
Scanner::enableBackups();
Scanner::setPathBackups("/my/path/backups");
Scanner::enableLiteMode();
Scanner::setAutoClean();

so you can use both.

Some methods are static because they are used by other classes that don't have the class instance.

So the anwser is yes, this is a "bad practice" and the class should be rewritten to use a real singleton instance (with a getInstance() method and newInstance() or something like that) and convert all static properties and static methods to non-static and then change the setters from return new self(); to return $this;. After this should be changed all methods called statically from the others classes and change it with the getInstance() method.

About the following lines:

PHP-Antimalware-Scanner/src/Scanner.php

Line 322 in f17ae16

self::setSilentMode();

PHP-Antimalware-Scanner/src/Scanner.php

Line 1685 in f17ae16

return new self();

for sure it is a big problem and I can fix it and I will release it in the next version.

[chrisdeeming]

Thank you for the detailed reply and quick fix for the infinite loop issue @marcocesarato.

My IDE, PhpStorm, seems to struggle with arrow accessors on static methods and a combination with the other issues I was experiencing led me to believe that it wasn't working.

I think I've identified a couple of other bugs which I will mention in separate issues.