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 to handle multiple # in vocab URIs. #1242

Open
wants to merge 1 commit into
base: skosmos-2
Choose a base branch
from

Conversation

pulquero
Copy link

No description provided.

Copy link
Collaborator

@kinow kinow left a comment

Choose a reason for hiding this comment

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

I think this one needs a test case (which would serve as example of a use case too)

@osma
Copy link
Member

osma commented Nov 10, 2021

Thanks for the PR @pulquero !

Can you explain what the problem is? As I understood it, you have URIs with more than one hash character (why?) and these break the current getId() method.

Like @kinow said above, a test case would be very good to have so that we know what exactly is being fixed here.

I wouldn't be surprised if such URIs caused problems elsewhere too - I don't recall encountering more than one hash character in URIs.

@pulquero
Copy link
Author

It is uncommon but still valid. Pushed testcase.

@sonarcloud
Copy link

sonarcloud bot commented Nov 10, 2021

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

Copy link
Collaborator

@kinow kinow left a comment

Choose a reason for hiding this comment

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

Thanks for the test @pulquero .

Not sure if this is a valid URL. Had a look at some SO with links to MDN and RFC's, but it looks like some browsers still try to handle the multiple hashes.

I opened a new Firefox tab, and typed about:config, then in the console window.location.hash was empty. Navigated to about:config#test, now the hash is #test, then about:config#test#prueba and now the has is #test#prueba.

In this test the hash/fragment appears to be multiHashVocabId. Can't say whether that's correct or not, but maybe somebody else can find another document or example to support one way or another 👍

@pulquero
Copy link
Author

I looked into it a bit deeper, and yes, multiple hashes are not valid. What I meant to do was #foo/bar, so I fixed that up on my side.

With regards to this PR, I'm happy to withdraw it. But, maybe the existing behaviour should be changed as it neither throws an error that the input is invalid or gracefully does something that is unsuprising. Four options to be considered then:
(1) Withdraw the PR
(2) Throw an error
(3) Behaviour in this PR (with comments added making it clear that iri is technically invalid)
(4) Take everything after the first #.

@kinow
Copy link
Collaborator

kinow commented Nov 11, 2021

I looked into it a bit deeper, and yes, multiple hashes are not valid. What I meant to do was #foo/bar, so I fixed that up on my side.

Thank you, it was interesting reading about it too (I never considered the possibility of multiple hashes.) Here's an interesting thread I found int he W3C list archives, though not conclusive 1

With regards to this PR, I'm happy to withdraw it. But, maybe the existing behaviour should be changed as it neither throws an error that the input is invalid or gracefully does something that is unsuprising. (...)

Sounds reasonable. Will leave it up to others, but I guess we could at least log a warn somewhere, if the decision is to not modify the code to handle the multiple hashes — no strong opinion from me.

Thanks!

Footnotes

  1. https://lists.w3.org/Archives/Public/public-rdf-in-xhtml-tf/2008Nov/0043.html

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.

3 participants