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

Use metadata search as a facet - 7.x-ISLANDORA-1425 #37

Merged
merged 6 commits into from
Oct 15, 2015
Merged

Use metadata search as a facet - 7.x-ISLANDORA-1425 #37

merged 6 commits into from
Oct 15, 2015

Conversation

whikloj
Copy link
Member

@whikloj whikloj commented Sep 11, 2015

Addresses ISLANDORA-1425

@whikloj
Copy link
Member Author

whikloj commented Sep 11, 2015

@adam-vessey I'd appreciate a look over this if you have a chance, as it was your code originally. In case I am missing something.

@whikloj
Copy link
Member Author

whikloj commented Sep 11, 2015

@bradspry Can you also have a look at this?

@bradspry
Copy link

@whikloj Tested your fix, it absolutely does work, but I recommend switching from urlencode() to rawurlencode() on line 239, to truly urlencode space characters as %20, per RFC 3986.

'!value' => rawurlencode(islandora_solr_facet_escape($original_value)),
));
return t('<a href="@url">@val</a>', array(
'@url' => url("islandora/search/") . '?type=dismax&f[0]=' . $solr_query,
Copy link
Member

Choose a reason for hiding this comment

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

Any particular reason this being changed to be a facet query?

@whikloj
Copy link
Member Author

whikloj commented Sep 14, 2015

@jordandukart it seemed to make it easier to deal with the internal data from the MODS record in much the same way it is handled in the faceting. The whole issue comes around the escaping of all the crazy stuff you get in MODS records you don't get people typing in a simple search box.

That said I'm not tied to it, but it worked.

@whikloj
Copy link
Member Author

whikloj commented Oct 5, 2015

@adam-vessey, does this look good to you?

'!value' => urlencode(islandora_solr_replace_slashes(islandora_solr_lesser_escape($original_value))),
'!value' => islandora_solr_facet_escape($original_value),
));
return t('<a href="@url">@val</a>', array(
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just use l()?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because that seemed to take me back to the original problem of double encoding of some characters.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is that referring to previous experimentation, appending to the path parameter itself in different ways?

l() effectively degrades to this (assuming links aren't being taken over in the theme):

function l($text, $path, array $options = array()) {
  // [... figure out $use_theme and handle "active" class ...] 
  if ($use_theme) {
    return theme('link', array('text' => $text, 'path' => $path, 'options' => $options));
  }
  // The result of url() is a plain-text URL. Because we are using it here
  // in an HTML argument context, we need to encode it properly.
  return '<a href="' . check_plain(url($path, $options)) . '"' . drupal_attributes($options['attributes']) . '>' . ($options['html'] ? $text : check_plain($text)) . '</a>';
}

@ in t()/format_string() == check_plain()

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, yes it was. Fair point, I have updated to use l() instead of url().

@adam-vessey
Copy link
Contributor

Looks good.

@whikloj
Copy link
Member Author

whikloj commented Oct 6, 2015

Thanks @adam-vessey, @bradspry do you want to confirm this solves the problem you experienced?

@ruebot
Copy link
Member

ruebot commented Oct 15, 2015

@bradspry can you confirm for @whikloj?

@bradspry
Copy link

@ruebot, @whikloj, I apologize, I did not catch the comment 9 days ago! Tested just now and all is well! (y)

Subsequent facet links work, everything is rawurlencoded(), its a beautiful thing:

/islandora/search?type=dismax&f[0]=mods_subject_name_personal_namePart_ms%3AKing%2C\%20Martin\%20Luther%2C\%20Jr.%2C\%201929\-1968&f[1]=mods_subject_topic_ms%3A%22African\%20Americans\-\-Social\%20conditions%22

@ruebot
Copy link
Member

ruebot commented Oct 15, 2015

Thanks @bradspry!

@whikloj can you do a 7.x-1.6 pull request? I'll merge this one now.

ruebot added a commit that referenced this pull request Oct 15, 2015
Use metadata search as a facet - 7.x-ISLANDORA-1425
@ruebot ruebot merged commit 733f7ea into Islandora:7.x Oct 15, 2015
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.

5 participants