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.

Sign up for GitHub

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

Jump to bottom

Fix: internal link misrepresented and misused #539

Merged
merged 9 commits into from Nov 17, 2019
Merged

Fix: internal link misrepresented and misused #539

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
f982ed8
fix: change slash_link definition from pipe
Sep 30, 2019
8958043
chore: remove redundant if-condition in html fun
Sep 30, 2019
274b4b5
chore: return nil from 'path' if it's empty
Oct 1, 2019
1b5178d
chore: no need to check file_path if path is empty
Oct 1, 2019
799867f
test: add tests for slash internal urls
Oct 1, 2019
baf8ee6
chore: remove test debugging
Oct 1, 2019
d9b1095
fix: enable path to remain an empty string
Oct 2, 2019
d324f3b
Merge branch 'master' into fix/internal-link-misrepresented-and-misused
gjtorikian Nov 17, 2019
85b67a0
Satisfy the cop
gjtorikian Nov 17, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 3 additions & 3 deletions lib/html-proofer/check/links.rb
View file
Open in desktop
Expand Up @@ -49,7 +49,7 @@ def run
next if @link.respond_to?(:rel) && @link.rel == 'dns-prefetch'
add_to_external_urls(@link.href)
next
elsif !@link.internal? && !@link.exists?
elsif !@link.remote? && !@link.exists?
Copy link
Owner

Choose a reason for hiding this comment

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

Wouldn't this be best as @link.internal? (no !)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would. I tried it and it created some more errors failing. Looks to be some discrepancy there. I will give another go.

add_issue("internally linking to #{@link.href}, which does not exist", line: line, content: content)
end

Expand Down Expand Up @@ -79,15 +79,15 @@ def check_schemes(link, line, content)
end

def handle_mailto(link, line, content)
if link.path.empty?
if link.path.nil?
Graborg marked this conversation as resolved.
Show resolved Hide resolved
add_issue("#{link.href} contains no email address", line: line, content: content)
elsif !link.path.include?('@')
add_issue("#{link.href} contains an invalid email address", line: line, content: content)
end
end

def handle_tel(link, line, content)
add_issue("#{link.href} contains no phone number", line: line, content: content) if link.path.empty?
add_issue("#{link.href} contains no phone number", line: line, content: content) if link.path.nil?
end

def handle_hash(link, line, content)
Expand Down
13 changes: 6 additions & 7 deletions lib/html-proofer/element.rb
View file
Open in desktop
Expand Up @@ -73,7 +73,7 @@ def parts
end

def path
Addressable::URI.unencode parts.path unless parts.nil?
Addressable::URI.unencode parts.path unless (parts.nil? || parts.path.empty?)
end

def hash
Expand Down Expand Up @@ -137,7 +137,7 @@ def external?
!internal?
end

# path is an anchor or a query
# path is an anchor, a query or refers to root
def internal?
hash_link || param_link || slash_link
end
Expand All @@ -151,11 +151,11 @@ def param_link
end

def slash_link
url.start_with?('|')
url.start_with?('/')
Graborg marked this conversation as resolved.
Show resolved Hide resolved
end

def file_path
return if path.nil?
return if path.nil? || path.empty?

path_dot_ext = ''

Expand All @@ -174,7 +174,6 @@ def file_path
end

file = File.join base, path

if @check.options[:assume_extension] && File.file?("#{file}#{@check.options[:extension]}")
file = "#{file}#{@check.options[:extension]}"
elsif File.directory?(file) && !unslashed_directory?(file) # implicit index support
Expand Down Expand Up @@ -221,9 +220,9 @@ def base

def html
# If link is on the same page, then URL is on the current page so can use the same HTML as for current page
if (hash_link || param_link) && internal?
if (hash_link || param_link)
@html
elsif slash_link && internal?
elsif slash_link
Graborg marked this conversation as resolved.
Show resolved Hide resolved
# link on another page, e.g. /about#Team - need to get HTML from the other page
create_nokogiri(absolute_path)
end
Expand Down
10 changes: 10 additions & 0 deletions spec/html-proofer/fixtures/links/broken_root_link_internal.html
View file
Open in desktop
@@ -0,0 +1,10 @@
<html>

<body>
<p>
Blah blah blah.
<a href="/broken_root_link_internalz.html">Not a real link!</a>
</p>
</body>

</html>
10 changes: 10 additions & 0 deletions spec/html-proofer/fixtures/links/working_root_link_internal.html
View file
Open in desktop
@@ -0,0 +1,10 @@
<html>

<body>
<p>
Blah blah blah.
<a href="/broken_root_link_internal.html">Not a real link!</a>
</p>
</body>

</html>
12 changes: 12 additions & 0 deletions spec/html-proofer/links_spec.rb
View file
Open in desktop
Expand Up @@ -79,6 +79,18 @@
expect(proofer.failed_tests.first).to match(%r{internally linking to .\/notreal.html, which does not exist})
end

it 'fails for broken internal root links' do
broken_root_link_internal_filepath = "#{FIXTURES_DIR}/links/broken_root_link_internal.html"
proofer = run_proofer(broken_root_link_internal_filepath, :file)
expect(proofer.failed_tests.first).to match(%r{internally linking to \/broken_root_link_internalz.html, which does not exist})
end

it 'succeeds for working internal root links' do
broken_root_link_internal_filepath = "#{FIXTURES_DIR}/links/working_root_link_internal.html"
proofer = run_proofer(broken_root_link_internal_filepath, :file)
expect(proofer.failed_tests).to eq []
end

it 'fails for link with no href' do
missing_link_href_filepath = "#{FIXTURES_DIR}/links/missing_link_href.html"
proofer = run_proofer(missing_link_href_filepath, :file)
Expand Down