search

dpovshed / octopus

Sitemap checker/stress test tool based on ReactPHP
11 stars 1 forks source link

Use react flux #31

Closed holtkamp closed 5 years ago

holtkamp commented 5 years ago

Been using this for a while in production now and it works nice. @dpovshed sorry for the mess of commits, for some reason the re-basing went a bit strange...

dpovshed commented 5 years ago

Hmm it works in general, but when I run my test with targetList as a plaintext I am getting

[error] Failed instantiating SimpleXMLElement:String could not be parsed as XML

holtkamp commented 5 years ago

Thanks for having a look. Yeah, you are right, I noticed also that loading the URLs from a plain text file broke. Already got the required changes ready, will push it coming week.

Additionally we should consider creating some tests to prevent such regressions...

Also worked on an improvement of logging progress in a table, will create a separate PR for this!

Op do 28 feb. 2019 21:10 schreef Dennis Povshedny <notifications@github.com

:

@dpovshed requested changes on this pull request.

Using react-flux - looks like this part is fine.

However, there are two problems:

  • this PR have also major rework of loading sitemap, maybe related to supporting multiple sitemaps (?)
  • during this rework very important for me functionality has been just lost - I need load URLs from a text file even more often than xml processing.

The TargetManager class has been deleted and Sitemap\Loader was introduced. But Target could be just a list of URLs.

As a reference - an excerpt of TargetManager::populate function

` switch ($this->config->targetType) { case 'xml': $xmlElement = new SimpleXMLElement($data); if ($this->isXmlSitemapIndex($xmlElement)) { $this->processSitemapIndex($xmlElement);

            return \count($this->queuedUrls);
        }
        if ($this->isXmlSitemap($xmlElement)) {
            $this->processSitemapElement($xmlElement);

            return \count($this->queuedUrls);
        }

        $mask = "/\<loc\>(.+)\<\/loc\>/miU";
        break;
    case 'txt':
        $mask = "/^\s*((?U).+)\s*$/mi";
        break;
    default:
        throw new Exception('Unsupported file type: '.$this->config->targetType);
}

`

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/dpovshed/octopus/pull/31#pullrequestreview-209285193, or mute the thread https://github.com/notifications/unsubscribe-auth/AAvY1XVMJztrgSt1BwNSLJZiZd1C5z6Bks5vSDekgaJpZM4bVqMB .

holtkamp commented 5 years ago

Submitted a cleaner PR which maintains use of TargetManager in https://github.com/dpovshed/octopus/pull/32 => closing this PR