[dev] [commit] r914 - in wwwbase/Crawler: . ParsedText RawPage

Mon Jul 29 17:05:43 EEST 2013

Gata, m-am pus serios pe treabă. :-) Am tone de feedback, dar sunt mai mult
chestii de principiu și de bune practici, nimic serios de întors pe dos în
cod.

*Generalități*
*
*
M-aș simți mai bine dacă în wwwbase/ stau doar chestiile care trebuie să
fie accesibile în browser (monitorul). Restul poate sta direct în rădăcină
(DEX/crawler) sau eventual în DEX/tools/crawler.
*

AbstractCrawler.php*

- include-urile le-aș pune relativ la  __DIR__. Vezi exemple prin tools/,
să zicem tools/migration.php. Îmi închipui că vom rula crawlerul din linia
de comandă, iar asta se poate întâmpla din orice director (rădăcină sau
wwwbase/ sau wwwbase/Crawler/). Nenorocirea cu include și require în PHP
este că se uită relativ la CWD, nu relativ la directorul scriptului.

- teoretic util.php le include pe toate celelalte și execută singur
db_init(), deci n-ar fi nevoie de ele.

- liniile 51 și 55 sunt identice, există vreun motiv?

- linia 59: curl nu raportează el codul de eroare? Poate e mai bine decât
să punem noi din burtă 404. Sau măcar un comentariu în care să explici de
ce punem noi 404.

- linia 81: cred că înțeleg apelul la strstr, dar ar merita un comentariu.
Vrei să salvezi doar linkurile către un subdirector, nu? Oricum, ar fi mai
clar:

if (StringUtil::startsWith($url, $this->currentLocation)) {
  $urlHash...
  $domain...
  saveLink(...)
}

și scăpăm și de returnul urât în mijlocul funcției. :-)

- liniile 83 și 166: ai un typo, urlResouce. :-)

- saveLink2DB, savePage2DB: aș face metodele astea constructori (sau metode
statice) în clasele respective, Link și CrawledPage, pe care le-aș crea în
phplib/models. Parcă e încapsularea mai bună în felul ăsta. Aș redenumi
Link în CrawledLink, că mai avem câteva obiecte cu Link în titlu. Structura
minimă a acestor clase ar fi:

class CrawledLink extends BaseObject implements DatedObject {
  public static $_table = 'CrawledLink';

  ...
}

Și cu ocazia asta aș trece de la idiorm la paris, adică în loc de
ORM::for_table("Link")->create() aș folosi
Model::factory('CrawledLink')->create();

- linia 132: sunt 99% sigur că id-ul unei pagini proaspăt salvate este
populat în câmpul id (sau care-o fi primary key), deci poți zice mai simplu
$this->currentPageId = $tableObj->id;

- linia 144: strstr nu e safe aici, căci URL-ul poate fi relativ și poate
conține http undeva în interior. Aș folosi StringUtil::startsWith($url,
'http');

Întrebare random: cum te descurci cu link-urile de tip mailto: sau alte
tipuri? Poate ar fi util să verifici explicit că protocolul este unul din
http sau https.

- linia 157: idem, bucata asta nu e safe, căci poți avea URL-uri dinamice:
index.php?foo=bar. Aș pune explicit condiția String::endsWith($url,
'index.php') sau, mai compact, aș face un regexp de genul

preg_match_all("/^(.*)/index.(php|asp|htm|html)$/", $liteURL, $matches);

după care dă-i un var_dump($matches) și vezi că una din componente îți dă
exact prima paranteză (partea până la ultimul slash).

- linia 179: nu îmi este clar ce rol are $justStarted, poți să-mi explici?

- liniile 185-197: este nevoie de blocul try/catch? Eu aș face așa (deși
știu că uneori tind să fiu neglijent):

  $nextLink = (string)ORM::for_table('Link')->raw_query("Select
canonicalUrl from Link where canonicalUrl not in (Select url from
CrawledPage);")->find_one()
  return $nextLink ? $nextLink->canonicalUrl : null;

*Crawler.php*
*
*
- linia 45: Deocamdată rămâne cum e, dar ar trebui modificată într-o
iterație ulterioară. Trebuie să controlăm mai fin, de la site la site, ce
are voie crawlerul să facă. Văd două configurări:

- zona în care crawlerul are voie să intre (whitelist și/sau blacklist)
- patternul paginilor pe care crawlerul are voie să le salveze (regexp
whitelist).

De exemplu, poate crawlerul are voie să intre prin tot site-ul, mai puțin
în zona de media. Din zonele permise, salvăm doar unele pagini. Din
celelalte extragem linkurile, dar nu le salvăm. Bunăoară, nu ne interesează
paginile cu indexuri de articole (pagina principală a unui site de știri),
ci doar articolele în sine (regexp /articol/.*.htm).

- în while(1) va fi nevoie de o temporizare la nivel de site (tot într-o
versiune ulterioară). Probabil e de ajuns un hash map de la domeniu la
timestamp-ul ultimei crawlări pe acel domeniu. Apoi getNextLink() va ști să
caute numai în domeniile pe care este ok să le crawleze. Asta va înlocui
apelul de sleep: dacă crawlerul are destule site-uri de crawlat încât să
nu-l agaseze pe niciunul din ele, poate nu are nevoie să doarmă.

- linia 108: hm, acum îmi dau seama: tu vrei ca o instanță de Crawler să
fie dedicată pentru un site, iar apoi să rulezi mai multe crawlere în mai
multe threaduri? E ok și așa, sau putem face ca mai sus (un crawler care
este și manager).

*ParsedText și RawPage*
*
*
Astea n-au ce căuta în repository. :-) Dacă vrei să le dedici niște
directoare în cadrul repository-ului, este ok, dar atunci

(1) nu le-aș pune în wwwbase/. Poate în docs/crawler? sau, cum ziceam, mută
tot directorul crawler în DEX/.
(2) în acele directoare, rulezi "svn propedit svn:ignore ." și pui "*".
Atunci poți avea oricâte fișiere acolo și svn nu o să se agite.

*clean_all.php*
*
*
- pentru concizie, definește te rog o constantă $nl
= pref_getSectionPreference('crawler', 'new_line'). Sau poți folosi direct
PHP_EOL. Nici nu știam de ea. :-)
*
*
- aceleași observații legate de include-uri (util.php este suficient)

- liniile 14 și 20: ar trebui să folosești valorile definite în dex.conf,
nu constante hard-coded

- pentru accesul la baza de date, cred că poți folosi direct db_execute(),
definit de noi în phplib/db.php.

- îmi place că scriptul e idempotent (dacă îl rulezi de două ori la rând nu
strică).

*cookie_jar și crawler_log*
*
*
pe astea le-aș pune în /tmp, sau eventual cookie_jar în /tmp și crawler_log
în directorul nostru log/. În general, aș evita să pun în repository tot ce
nu este identic pentru toată lumea.

*simple_html_dom.php*
*
*
l-aș muta în phplib/, chiar dacă momentan nu este folosit decât de către
crawler. Bănuiesc că nu l-ai modificat, nu? Că încercăm să ne educăm să
păstrăm bibliotecile externe intacte și să punem în fișiere separate ce
customizăm noi.

2013/7/27 <automailer at dexonline.ro>

> Author: alinu
> Date: Sat Jul 27 22:36:45 2013
> New Revision: 914
>
> Log:
> Prima versiune de Crawler
>
> Added:
>    wwwbase/Crawler/
>    wwwbase/Crawler/.htaccess
>    wwwbase/Crawler/AbstractCrawler.php
>    wwwbase/Crawler/AppLog.php
>    wwwbase/Crawler/Crawler.php
>    wwwbase/Crawler/ParsedText/
>    wwwbase/Crawler/ParsedText/2013-07-27 22_20_52
>    wwwbase/Crawler/ParsedText/2013-07-27 22_21_23
>    wwwbase/Crawler/ParsedText/2013-07-27 22_21_53
>    wwwbase/Crawler/ParsedText/2013-07-27 22_22_23
>    wwwbase/Crawler/ParsedText/2013-07-27 22_22_54
>    wwwbase/Crawler/ParsedText/2013-07-27 22_23_24
>    wwwbase/Crawler/ParsedText/2013-07-27 22_23_55
>    wwwbase/Crawler/ParsedText/2013-07-27 22_24_25
>    wwwbase/Crawler/ParsedText/2013-07-27 22_24_55
>    wwwbase/Crawler/ParsedText/2013-07-27 22_25_26
>    wwwbase/Crawler/ParsedText/2013-07-27 22_25_56
>    wwwbase/Crawler/ParsedText/2013-07-27 22_26_14
>    wwwbase/Crawler/ParsedText/2013-07-27 22_26_44
>    wwwbase/Crawler/ParsedText/2013-07-27 22_27_15
>    wwwbase/Crawler/ParsedText/2013-07-27 22_27_46
>    wwwbase/Crawler/ParsedText/2013-07-27 22_28_16
>    wwwbase/Crawler/ParsedText/2013-07-27 22_28_47
>    wwwbase/Crawler/ParsedText/2013-07-27 22_29_17
>    wwwbase/Crawler/RawPage/
>    wwwbase/Crawler/RawPage/2013-07-27 22_20_52
>    wwwbase/Crawler/RawPage/2013-07-27 22_21_23
>    wwwbase/Crawler/RawPage/2013-07-27 22_21_53
>    wwwbase/Crawler/RawPage/2013-07-27 22_22_23
>    wwwbase/Crawler/RawPage/2013-07-27 22_22_54
>    wwwbase/Crawler/RawPage/2013-07-27 22_23_24
>    wwwbase/Crawler/RawPage/2013-07-27 22_23_55
>    wwwbase/Crawler/RawPage/2013-07-27 22_24_25
>    wwwbase/Crawler/RawPage/2013-07-27 22_24_55
>    wwwbase/Crawler/RawPage/2013-07-27 22_25_26
>    wwwbase/Crawler/RawPage/2013-07-27 22_25_56
>    wwwbase/Crawler/RawPage/2013-07-27 22_26_14
>    wwwbase/Crawler/RawPage/2013-07-27 22_26_44
>    wwwbase/Crawler/RawPage/2013-07-27 22_27_15
>    wwwbase/Crawler/RawPage/2013-07-27 22_27_46
>    wwwbase/Crawler/RawPage/2013-07-27 22_28_16
>    wwwbase/Crawler/RawPage/2013-07-27 22_28_47
>    wwwbase/Crawler/RawPage/2013-07-27 22_29_17
>    wwwbase/Crawler/clean_all.php
>    wwwbase/Crawler/cookie_jar
>    wwwbase/Crawler/crawler_log
>    wwwbase/Crawler/simple_html_dom.php
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://list.dexonline.ro/pipermail/dev/attachments/20130729/5ee9110d/attachment-0001.html>