Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactored the ContentBuilder out of the ContentExtractor #33

Merged
merged 4 commits into from
Jan 12, 2016

Conversation

bdunogier
Copy link
Contributor

Type: refactoring
Backward compatible: yes

Moves the logic used in ContentExtractor::buildSiteConfig() to a new method in the ContentBuilder. I have left the HostFingerPrint part in the ContentExtractor, as I'm not 100% sure what it does (feedback welcome).

This change makes it possible to build site config objects from anywhere using the host name, as done by http://github.com/wallabag/wallabag in the site login feature PR.

TODO

  • Fix tests, there are real failures (go over call logic in general)
  • Remove unrelated code about guzzle client

@bdunogier
Copy link
Contributor Author

Unsure about the message log test, will need to dig a bit deeper to fix the test.

@bdunogier
Copy link
Contributor Author

The tests pass locally, I'm a bit confused now. Could somebody test ?

@bdunogier
Copy link
Contributor Author

And the tests are passing.

@j0k3r
Copy link
Owner

j0k3r commented Jan 11, 2016

I'll have a look later this week 👌

@j0k3r
Copy link
Owner

j0k3r commented Jan 12, 2016

The HostFingerPrint check the given html and determine the website platform. In the default config, you have:

'fingerprints' => array(
    '/\<meta\s*content=\"blogger\"\s*name=\"generator\"/i' => 'fingerprint.blogspot.com',
    '/\<meta\s*name=\"generator\"\s*content=\"Blogger\"/i' => 'fingerprint.blogspot.com',
    '/\<meta\s*name=\"generator\"\s*content=\"WordPress/i' => 'fingerprint.wordpress.com',
),

Which mean, we are looking for that html content (meta blabla) and if we find it we'll apply the value corresponding as the host and then apply the config file (for example) .wordpress.com.txt. This way, even if the url doesn't contains wordpress.com we can apply a wordpress site config on the blog.

Hope I'm clear :)

@j0k3r
Copy link
Owner

j0k3r commented Jan 12, 2016

Looks good to me, I'll tweaked few things after merge.
Thanks !

Can't wait to the future steps in Wallabag :)

j0k3r added a commit that referenced this pull request Jan 12, 2016
Refactored the ContentBuilder out of the ContentExtractor
@j0k3r j0k3r merged commit 9027cd1 into j0k3r:master Jan 12, 2016
j0k3r added a commit that referenced this pull request Jan 12, 2016
Following #33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants