JayBizzle / Crawler-Detect

🕷 CrawlerDetect is a PHP class for detecting bots/crawlers/spiders via the user agent
https://crawlerdetect.io
MIT License
1.96k stars 255 forks source link

Simplified regex match storing code #398

Closed Bilge closed 4 years ago

Bilge commented 4 years ago

Rather than first writing matches to a temporary variable, we write them directly to the class member variable. This eliminates the temporary $matches variable and also the temporary $result variable.

Note that we also eliminated the if statement which is intentional because it is this author's belief that it is incorrect for two reasons:

  1. It should not be conditional whether $matches are saved; if the method is called then $matches should always be updated rather than persist the result from a previous call.
  2. As an aside, and although not addressed by this PR, it is this author's strong belief that storing transient data, such as regex matches, between method calls is an anti-pattern because it is not concurrency-safe. Such runtime information should be returned immediately by the method. However, this refactoring is left for future work and affects the entire class design, not just this method.
JayBizzle commented 4 years ago

I will add the following test to make sure $matches doesn't persist.

    /** @test */
    public function matches_does_not_persit_across_multiple_calls()
    {
        $this->CrawlerDetect->isCrawler('Mozilla/5.0 (iPhone; CPU iPhone OS 7_1 like Mac OS X) AppleWebKit (KHTML, like Gecko) Mobile (compatible; Yahoo Ad monitoring; https://help.yahoo.com/kb/yahoo-ad-monitoring-SLN24857.html)');
        $matches = $this->CrawlerDetect->getMatches();
        $this->assertEquals($this->CrawlerDetect->getMatches(), 'monitoring', $matches);

        $this->CrawlerDetect->isCrawler('This should not match');
        $matches = $this->CrawlerDetect->getMatches();
        $this->assertNull($this->CrawlerDetect->getMatches());
    }