When a view is created sparql_views_plugin_query_sparql.inc checks to make sure the query is valid (around line 48). There's an error on line 53 that causes an ugly error message and prevents user from getting to views screen if their end point is invalid or returns no data. The second argument in the t() function on line 53 should be an array. It's currently a string.

CommentFileSizeAuthor
#4 1150284_4_sv_t.patch1.99 KBscor
#1 fix_t_function_args-1150284.patch1.2 KBbryanhirsch

Comments

bryanhirsch’s picture

bryanhirsch commented
StatusFileSize
new fix_t_function_args-1150284.patch1.2 KB

The attached patch fixes the bug and cleans up these few lines of code.

Anonymous’s picture

Anonymous (not verified) commented
Status: Active » Needs work

Thanks for the patch :)

Two style issues:

  • It looks like you are using tabs to indent, which makes the patch contain 6-space indents. It should have 2-space indents.
  • You have broken the string in the t() function at 80 chars, but it is fine to leave it as one string. Here is an example from node.module
      if (isset($node->nid) && (node_last_changed($node->nid) > $node->changed)) {
    form_set_error('changed', t('The content on this page has either been modified by another user, or you have already submitted modifications using this form. As a result, your changes cannot be saved.'));
  }
Anonymous’s picture

Anonymous (not verified) commented
Status: Needs work » Fixed

The second argument to t() actually was an array already, but was hard to tell because of the length of the arguments in the function call. I went ahead and cleaned it up a little by moving the link to a variable as you had done.

Done in commit http://drupalcode.org/project/sparql_views.git/commit/702686f

scor’s picture

scor commented
Status: Fixed » Needs review
StatusFileSize
new 1150284_4_sv_t.patch1.99 KB

Lin, you've only moved the bug one line up :) When Bryan said "The second argument in the t() function on line 53 should be an array. It's currently a string.", he meant the second call to t().

However, usually the string is kept in one call to t() with the urls as replacements. It helps translators to have more context when doing the translations.

edit: sorry again for extra hunks...

Anonymous’s picture

Anonymous (not verified) commented
Title: Patch for bug that appears when end point is invalid or returns no data » Error when no SV resources are available for an endpoint

ok, I see. It wasn't as much that the second argument in the call to t() should be an array, it was that the parenthesis was misplaced and that there shouldn't have been a second argument.

You changed the ! to an @... is there a reason? I don't think we need to check_plain a url that is hardcoded, since it is sanitized by the fact that we wrote the path directly in code. The relevant docs:

!variable: Inserted as is. Use this for text that has already been sanitized.
scor’s picture

scor commented

@ is the placeholder for lazy developers who don't want to think twice whether a string is already sanitized or not (check core, you will see this same pattern very often). ! is valid here to for the reason you mention, but the overhead of the extra check_plain with @ is really negligible for something that is run once on a page that's rarely going to be displayed (since the error should be fixed by the site admin). I agree with your remark for the general case though.

Edit: so feel free to use either ! or @ :)