-
Notifications
You must be signed in to change notification settings - Fork 1
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
Store annotations relative to content, not content display #2
Conversation
oops, looks like I broke some stuff while separating out my code. hold off testing, update is on its way. |
@@ -166,6 +166,7 @@ function annotation_api_search() { | |||
$text = isset($_GET['text']) ? $_GET['text'] : NULL; | |||
$quote = isset($_GET['quote']) ? $_GET['quote'] : NULL; | |||
$uri = isset($_GET['uri']) ? $_GET['uri'] : NULL; | |||
$nid = isset($_GET['nid']) ? $_GET['nid'] : NULL; |
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.
I think that is better to use a check_plain() on this data.
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.
Thanks for the comment, Paolo.
I don't think we need sanitization here, but I could well be wrong.
In this particular line, nid is possibly used in a database query, but not returned in html. So check_plain won't do anything useful. And we're using the drupal database API, which does its own escaping/sanitization of query parameters.
Annotations are stored in the db without any html escaping. but when they are retrieved they go through drupal_json_output, which escapes HTML. [at least, if I put html tags into a comment, they come out displayed as text rather than interpreted as HTML].
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.
You are absolutely right, PDO already escapes arguments, sorry for the wrong suggestion.
Tested all this now, and basically got it to work. Nice work! :) I added a pull request with some small changes back, which you can merge into your branch. Before I merge this into master, here are some remaining issues that I found:
|
Node based annotations: small fixes
@tanius Is this multiple calls to the SAME search url, or to different ones?
|
@danohuiginn It's multiple calls to the same URI, like:
Could it have to do with the jQuery selector of the annotator plugin applying multiple times? Just a wild guess though. |
Found the reason for the multiple calls to Now I use So, this one's fixed. I'll put it into the docs. |
…tent display". Merges branch 'danohuiginn-node_based_annotations'. Conflicts: drupal_annotation/annotation.install
Merged what we have so far to. It's working as is, so I'll put in enhancement type issues for the fine tuning related above (no Also, I have put @danohuiginn's setup instructions into the Open Ethnographer manual. |
DESIGN
We store the appropriate node id (nid) with every annotation. We override the
annotation API to search by nid rather than URL. We also manipulate the url to
be always the path to the node detail page.
SETUP
[this selects the div immediately surrounding the node content. Any other selector should
also be fine, provided the markup within it will be identical on both node and index pages]
manually add the nid field