Open chrisdeeming opened 2 years ago
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();
.
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: https://github.com/marcocesarato/PHP-Antimalware-Scanner/blob/f17ae163ea4a1a553fe979127137c80911aaa26a/src/Scanner.php#L322 https://github.com/marcocesarato/PHP-Antimalware-Scanner/blob/f17ae163ea4a1a553fe979127137c80911aaa26a/src/Scanner.php#L1685 for sure it is a big problem and I can fix it and I will release it in the next version.
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.
First, documentation.
The README.md file contains a section about instantiating and using this programmatically:
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:
I guess the documentation needs to be something like this but you'd be instantiating the class at least 6 times:
However, the bigger problem with this approach is the
__construct
method if you're not running via the CLI:In case the issue isn't obvious, the
setSilentMode
method ends in: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:
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.
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?