Add support of http_header property in site config #61

Kdecherf · 2016-11-06T21:29:32Z

This PR adds the ability to graby to use user-agent provided in site configs instead of default_ua and user_agents from HttpClient.

http_header is extracted from site config and provided to HttpClient#fetch() and HttpClient#getUserAgent() to extract the user-agent, if any. In case of missing value we fall back to the original behavior: check host from user_agents or use default_ua.

There are some issues however, at this time:

~~default_ua is not used anymore as long as a http_header(user-agent) is currently provided in global.txt. A solution could be to ignore the user-agent provided by global, any thoughts?~~
HttpClient does not have access to SiteConfig, some tests began to fail because of the deletion of default user_agents array (e9f2981). I fixed them by providing a custom array with user_agents to the HttpClient instance in the test. Not sure if it's okay for you.

Fixes #49

coveralls · 2016-11-06T22:04:56Z

Coverage increased (+0.01%) to 98.523% when pulling 3f9b341 on Kdecherf:http-headers into 57e2757 on j0k3r:master.

Kdecherf · 2016-11-06T22:05:53Z

Well, nevermind for the first issue. It was a non-tracked manual change in vendor/j0k3r/graby-site-config/global.txt on my side. Anyway, I don't know if you are okay with this behavior: user_agents and default_ua are completely ignored if global.txt provides a http_header(user-agent), which is not the case actually (for now).

j0k3r

This look interesting, thanks for that !
I'll soon test it

j0k3r · 2016-11-06T22:19:14Z

src/Extractor/HttpClient.php

     * @param bool   $skipTypeVerification Avoid mime detection which means, force GET instead of potential HEAD
     *
     * @return array With keys effective_url, body & headers
     */
-    public function fetch($url, $skipTypeVerification = false)
+    public function fetch($url, $http_header = array(), $skipTypeVerification = false)


Can you use httpHeader instead?
All variables are lowerCamelCase.

j0k3r · 2016-11-06T22:22:35Z

src/SiteConfig/ConfigBuilder.php

@@ -341,6 +345,8 @@ public function parseLines(array $lines)
            } elseif ((substr($command, -1) == ')') && preg_match('!^([a-z0-9_]+)\((.*?)\)$!i', $command, $match) && $match[1] == 'replace_string') {
                array_push($config->find_string, $match[2]);
                array_push($config->replace_string, $val);
+            } elseif ((substr($command, -1) == ')') && preg_match('!^([a-z0-9_]+)\(([a-z0-9_-]+)\)$!i', $command, $match) && $match[1] == 'http_header' && in_array($match[2], array('user-agent'))) {


Instead of in_array($match[2], array('user-agent')) can you use a simple 'user-agent' === $match[2]?

I used in_array() to make addition of new headers easier. But I can replace it with a strict check if you prefer.

I will let in_array() for #62

coveralls · 2016-11-06T22:48:34Z

Coverage increased (+0.01%) to 98.523% when pulling 51a6852 on Kdecherf:http-headers into 57e2757 on j0k3r:master.

j0k3r · 2016-11-07T06:53:28Z

src/Extractor/HttpClient.php

-                '.wikipedia.org' => 'Mozilla/5.2',
-                '.fok.nl' => 'Googlebot/2.1',
-                'getpocket.com' => 'PHP/5.2',
-            ),


If we remove that, we should be sure that these values are in the config file for each domain.

Ok, skip it, I've checked, they are all already defined in the site config 👍

j0k3r · 2016-11-07T08:10:07Z

src/Extractor/HttpClient.php

     * @param bool   $skipTypeVerification Avoid mime detection which means, force GET instead of potential HEAD
     *
     * @return array With keys effective_url, body & headers
     */
-    public function fetch($url, $skipTypeVerification = false)
+    public function fetch($url, $httpHeader = array(), $skipTypeVerification = false)


Could you move that new parameter as third one? If we inject it in the second arg, it'll require to release a new major version because the signature of this public method will change. I know, it'll require to put the false argument for $skipTypeVerification every where you'll have to inject $httpHeader but it'll avoid BC

j0k3r · 2016-11-07T08:15:25Z

src/Extractor/HttpClient.php

    {
        $ua = $this->config['ua_browser'];
+
+        if (array_key_exists('user-agent', $httpHeader) && !empty($httpHeader['user-agent'])) {


You don't need the array_key_exists(), empty() already perform the isset().

j0k3r · 2016-11-07T08:21:32Z

src/Extractor/HttpClient.php

-                '.wikipedia.org' => 'Mozilla/5.2',
-                '.fok.nl' => 'Googlebot/2.1',
-                'getpocket.com' => 'PHP/5.2',
-            ),


Ok, skip it, I've checked, they are all already defined in the site config 👍

j0k3r · 2016-11-07T08:26:13Z

I fixed them by providing a custom array with user_agents to the HttpClient instance in the test. Not sure if it's okay for you.

Yeah it's ok, you should have an other option to inject it in the fetch() but what you've done allow the fallback to the old behavior to still be tested

Signed-off-by: Kevin Decherf <kevin@kdecherf.com>

A static array is added to whitelist authorized headers during parsing. Signed-off-by: Kevin Decherf <kevin@kdecherf.com>

These default options are no longer needed with the use of http_header(user-agent) Signed-off-by: Kevin Decherf <kevin@kdecherf.com>

Signed-off-by: Kevin Decherf <kevin@kdecherf.com>

coveralls · 2016-11-08T21:38:41Z

Coverage increased (+0.01%) to 98.532% when pulling b25abc6 on Kdecherf:http-headers into cef2fba on j0k3r:master.

j0k3r

👍

Improve tests from #61

j0k3r reviewed Nov 6, 2016

View reviewed changes

j0k3r added the enhancement label Nov 6, 2016

Kdecherf force-pushed the http-headers branch from 3f9b341 to 51a6852 Compare November 6, 2016 22:42

j0k3r reviewed Nov 7, 2016

View reviewed changes

j0k3r mentioned this pull request Nov 7, 2016

Handle referer from siteconfig #62

Closed

j0k3r reviewed Nov 7, 2016

View reviewed changes

j0k3r mentioned this pull request Nov 7, 2016

Add some user-agent fivefilters/ftr-site-config#218

Merged

Kdecherf added 6 commits November 8, 2016 22:25

Add support of SiteConfig->http_header in HttpClient::fetch()

74399f7

Signed-off-by: Kevin Decherf <kevin@kdecherf.com>

HttpClient: use user-agent from SiteConfig if it exists

0aa7a4c

Signed-off-by: Kevin Decherf <kevin@kdecherf.com>

HttpClient: add log in getUserAgent, update tests

7c5962b

Signed-off-by: Kevin Decherf <kevin@kdecherf.com>

ConfigBuilder: add support of http_header in mergeConfig

7ab649b

A static array is added to whitelist authorized headers during parsing. Signed-off-by: Kevin Decherf <kevin@kdecherf.com>

HttpClient: remove default user_agents config

806eba6

These default options are no longer needed with the use of http_header(user-agent) Signed-off-by: Kevin Decherf <kevin@kdecherf.com>

Fix failing testBuildSiteConfig

b25abc6

Signed-off-by: Kevin Decherf <kevin@kdecherf.com>

Kdecherf force-pushed the http-headers branch from 51a6852 to b25abc6 Compare November 8, 2016 21:31

j0k3r approved these changes Nov 9, 2016

View reviewed changes

j0k3r merged commit bdd9f7b into j0k3r:master Nov 9, 2016

Kdecherf mentioned this pull request Nov 9, 2016

Add support of referer from site config #64

Merged

Kdecherf deleted the http-headers branch November 14, 2016 10:15

j0k3r added a commit that referenced this pull request Nov 14, 2016

Merge pull request #65 from Kdecherf/post-61

21dbc17

Improve tests from #61

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support of http_header property in site config #61

Add support of http_header property in site config #61

Kdecherf commented Nov 6, 2016 •

edited

Loading

coveralls commented Nov 6, 2016

Kdecherf commented Nov 6, 2016 •

edited

Loading

j0k3r left a comment

j0k3r Nov 6, 2016

j0k3r Nov 6, 2016

Kdecherf Nov 6, 2016

Kdecherf Nov 7, 2016

coveralls commented Nov 6, 2016

j0k3r Nov 7, 2016

j0k3r Nov 7, 2016

j0k3r Nov 7, 2016

j0k3r Nov 7, 2016

j0k3r Nov 7, 2016

j0k3r commented Nov 7, 2016

coveralls commented Nov 8, 2016

j0k3r left a comment

Add support of http_header property in site config #61

Add support of http_header property in site config #61

Conversation

Kdecherf commented Nov 6, 2016 • edited Loading

coveralls commented Nov 6, 2016

Kdecherf commented Nov 6, 2016 • edited Loading

j0k3r left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Nov 6, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

j0k3r commented Nov 7, 2016

coveralls commented Nov 8, 2016

j0k3r left a comment

Choose a reason for hiding this comment

Kdecherf commented Nov 6, 2016 •

edited

Loading

Kdecherf commented Nov 6, 2016 •

edited

Loading