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

Fix for image.path causing an invalid url error #228

Merged
merged 1 commit into from
Aug 22, 2017

Conversation

benbalter
Copy link
Collaborator

If image.path is invalid, absolute_url? will return nil which is falsy. Explicitly look for false before we try to make the URL absolute to avoid an upstream error.

Copy link
Member

@DirtyF DirtyF left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests are passing, should be fine, but previous tests were also OK. 🤔
I just wonder if we should not provide another example of an invalid path. Whats does : stand for here?

@benbalter
Copy link
Collaborator Author

I should have been more explicit. I'm sorry. This is a fix for the failing test on master described in #225 (comment).

Whats does : stand for here?

Jekyll used to have issues with :s in paths, fixed in 3.5. When we call filters.absolute_url, that method doesn't guard against Addressable invalid URL error when it tries to parse the URL.

This was implemented originally via #199:

My intention here was to prevent the build from failing due to invalid or missing URLs. If a user provides an invalid URL right now, we output it to the best of our ability, even if it's wrong (or isn't live, etc.).

@DirtyF
Copy link
Member

DirtyF commented Aug 22, 2017

@benbalter crystal clear, thanks for the heads up.

@benbalter
Copy link
Collaborator Author

@jekyllbot: merge +minor

@benbalter benbalter merged commit ba3b326 into master Aug 22, 2017
@benbalter benbalter deleted the fix-for-invalid-url branch August 22, 2017 17:02
benbalter added a commit that referenced this pull request Aug 22, 2017
@jekyll jekyll locked and limited conversation to collaborators Apr 30, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants