-
Notifications
You must be signed in to change notification settings - Fork 26
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
Enhancement: Consider baseURI of referringElement (<base href="...">) #109
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great thanks! Would be great if we could also add a test for this scenario in this test case:
class CrawlCommandTest extends EndToEndTestCase |
Maybe by setting up a new "test website" in tests/Example
e.g. tests/Example/base-url
lib/Model/ReferringElement.php
Outdated
@@ -8,6 +8,7 @@ class ReferringElement | |||
{ | |||
private $xpath = ''; | |||
private $title = ''; | |||
private $baseURI = ''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Camel case would be more consistent $baseUri
lib/Model/ReferringElement.php
Outdated
|
||
return $new; | ||
} | ||
|
||
public function baseURI(): ?string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same
lib/Model/Url.php
Outdated
$base = self::fromUrl($baseURI); | ||
} else { | ||
$base = $this; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe extract to a private method resolveBaseUri
and remove the else
?
Thank you for all your hints! I tried to make all of these things. But I couldn't get the unit tests to run successfully on my machine. I think, however, my added test should work well. (don't know whether it's already automatically checked here). |
Nice work, thanks! The test works fine on Travis and on my local machine, strange that it doesn't on yours 🤔 |
Maybe I haven't installed it correctly. If I do this:
then I get many errors. It says the command "bin/fink" doesn't exist. If I call "bin/fink" directly:
it works well 🤷♂️ Anyway, thanks for the merge! I'm happy I could help! 😊 ... was my first time making a pull request 😊 |
#108