-
Notifications
You must be signed in to change notification settings - Fork 95
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
Fixes for searches, ajax, waypoints, easyrdf HTTP client errors #1197
Conversation
…ch queries; fixes waypoint system used in search; added checks for various easyrdf http client errors (cherry picked from commit 9f762ff)
Codecov Report
@@ Coverage Diff @@
## master #1197 +/- ##
============================================
- Coverage 68.71% 68.26% -0.46%
- Complexity 1598 1609 +11
============================================
Files 32 32
Lines 3929 3955 +26
============================================
Hits 2700 2700
- Misses 1229 1255 +26
Continue to review full report at Codecov.
|
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.
Added a few notes on TODO items.
More generally, we need a way to test the changes in this PR, to verify that the fixes are indeed working as they should.
if ($this->model->getConfig()->getLogCaughtExceptions()) { | ||
error_log('Caught exception: ' . $e->getMessage()); | ||
} | ||
$this->invokeGenericErrorPage($request, $e->getMessage()); |
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.
Generic error page is no longer used here, as the search result page can now show a proper warning message.
$results = $exsparql->queryLabel($exuri, $lang); | ||
|
||
return isset($results[$lang]) ? $results[$lang] : null; | ||
try { |
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.
Here the idea is to catch errors accessing labels from another vocabulary.
$exsparql = $exvoc->getSparql(); | ||
$results = $exsparql->queryNotation($exuri); | ||
return isset($results) ? $results : null; | ||
try { |
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.
Same as above, but accessing notations from external vocabularies.
return $vocab; | ||
} else { | ||
// not found in preferred vocabulary, fall back to next method | ||
try { |
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.
Idea here is to handle errors that happen when disambiguating between URI namespaces (e.g. is this URI from YSO or YSO Places?)
@@ -483,8 +490,16 @@ private function disambiguateVocabulary($vocabs, $uri, $preferredVocabId = null) | |||
|
|||
// no preferred vocabulary, or it was not found, search in which vocabulary the concept has a label | |||
foreach ($vocabs as $vocab) { | |||
if ($vocab->getConceptLabel($uri, null) !== null) | |||
return $vocab; | |||
try { |
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.
Same as above, catch errors that happen when disambiguating URIs
@@ -210,7 +210,8 @@ function loadLimitedResults(parameters) { | |||
clearResultsAndAddSpinner(); | |||
$.ajax({ | |||
data: parameters, | |||
success : function(data) { | |||
complete : function(jqXHR, textStatus) { |
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.
This callback will be executed in any case, both for successful and unsuccesful requests.
…g found by sonarqube)
…er can't do anything about it)
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
I think this is good enough now - tests are missing, but handling errors like this are a bit challenging to test anyway. I'll merge this and if anything turns up it can be fixed in subsequent PRs. |
This is the last part of fixes split off from PR #1033. I created a new branch from master (after merging PRs #1194 #1195 #1196) and cherry-picking the last commit in PR #1033 originally created by @kouralex . Some merge conflicts were fixed, hopefully correctly - in
headerbar.twig
I simply discarded the change as the template had been subsequently modified in PR #1061 (adding accessibility features) and it didn't seem to apply anymore.Original description of changes from the commit message: