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

[Core: Module] Fix errors in 500-reponse logic #4193

Merged
merged 3 commits into from
Dec 17, 2018
Merged

[Core: Module] Fix errors in 500-reponse logic #4193

merged 3 commits into from
Dec 17, 2018

Conversation

johnsaigle
Copy link
Contributor

@johnsaigle johnsaigle commented Dec 11, 2018

Brief summary of changes

Instrument List crashes on bugfix because the $user variable was not properly set. A TypeError is thrown by responseStatus500() as a result.

This isn't clear from the code changes but this should fix errors like:
Fatal error: Uncaught TypeError: Argument 2 passed to Module::responseStatus500() must be an instance of User, null given, called in /home/travis/build/aces/Loris/php/libraries/Module.class.inc on line 309 and defined in /home/travis/build/aces/Loris/php/libraries/Module.class.inc:334

@johnsaigle johnsaigle added Cleanup PR or issue introducing/requiring at least one clean-up operation Testing PR contains test plan or automated test code (or config files for Travis) [branch] bugfix labels Dec 11, 2018
@johnsaigle johnsaigle added the State: Needs work PR awaiting additional work by the author to proceed label Dec 11, 2018
@johnsaigle johnsaigle changed the title [Core: Module] Fix TypeError caused by unset User [Core: Module] Fix errors in 500-reponse logic Dec 11, 2018
@johnsaigle
Copy link
Contributor Author

johnsaigle commented Dec 11, 2018

In fixing the above error, instrument_list stopped working because of the way it handles its routing.

A 500 error would appear on the front-end and this exception was in the error log:
LorisException: Page requires a timepoint to load in /var/www/loris/modules/instrument_list/php/instrument_list.class.inc:85

as the code throws a LorisException when TimePoint or Candidate are missing from the request. However, this Exception ended up being caught by Module after #4163 was merged.

This code was changed to throw a NotFound exception instead so that the module routing code does not get returned.

If any of that was hard to understand it's worth reviewing instrument_list/php/module.class.inc and the handle function in php/libraries/Module.class.inc

@@ -82,11 +82,11 @@ class Instrument_List extends \NDB_Menu_Filter
$user =& \User::singleton();
$timePoint = $this->timePoint;
if ($timePoint === null) {
throw new \LorisException("Page requires a timepoint to load");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These were being caught by https://github.com/aces/Loris/pull/4163/files#diff-b9919f169806c905a8606fd31442044fR304, preventing instrument_list from loading.

@johnsaigle johnsaigle removed the State: Needs work PR awaiting additional work by the author to proceed label Dec 12, 2018
@driusan
Copy link
Collaborator

driusan commented Dec 13, 2018

@kongtiaowang can you manually test this?

@xlecours
Copy link
Contributor

I think the $user parameter of the responseStatus500 function could be retrieved from the request instead of being injected.
Also, responseStatus500 should use the AnonymousPageDecorationMiddleware or UserPageDecorationMiddleware according to the class of user.
Arguably, the resulting response should be HTTP/1.1 400 Bad Request if the request is missing parameters.

@kongtiaowang kongtiaowang added the Passed manual tests PR has been successfully tested by at least one peer label Dec 17, 2018
@driusan
Copy link
Collaborator

driusan commented Dec 17, 2018

Merging despite @xlecours's comment since the instrument list page needs the bug fix, and the feedback could be done in another PR..

@driusan driusan merged commit f58a249 into aces:bugfix Dec 17, 2018
@ridz1208 ridz1208 added this to the 20.1.2 milestone Dec 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Cleanup PR or issue introducing/requiring at least one clean-up operation Passed manual tests PR has been successfully tested by at least one peer Testing PR contains test plan or automated test code (or config files for Travis)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants