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 New Router #3584

Merged
merged 92 commits into from
May 9, 2018
Merged

Use New Router #3584

merged 92 commits into from
May 9, 2018

Conversation

driusan
Copy link
Collaborator

@driusan driusan commented Apr 12, 2018

This updates LORIS to use the PSR15 RequestHandler/Middleware based router that was implemented in PRs #3477, #3480, and #3513.

Most of the mod_rewrite rules, main.php, and NDB_Caller are replaced by a new index.php, which generates a PSR7 ServerRequestInterface and delegates to the module's Module handler.

AjaxHelper.php remains, but is deprecated, as existing scripts will need to be updated to be proper endpoints in the module's router. (Once this is done, the separate router.php for PHP's built in web server can also be removed, and mod_rewrite dependency for LORIS can be removed.)

main.php and NDB_Caller also remain for now in order to load instruments. Once an instrument module is implemented to handle the instruments routers, they can be removed.

Note that instrument URLs also change from $LORIS/instrumentname to $LORIS/instruments/instrumentname as part of this PR.

@driusan driusan requested a review from xlecours April 30, 2018 14:35
@driusan
Copy link
Collaborator Author

driusan commented Apr 30, 2018

@PapillonMcGill I think I've addressed the issues with the data team helper module, but I'm not familiar enough with the module and the test plan doesn't have enough detail to know if it's working properly (but the links are definitely working better than before.)

@xlecours @ridz1208 This is now passing Travis (Finally!), so it can be reviewed.

@PapillonMcGill
Copy link
Contributor

@driusan Datahelper now report that "This page (pagename) is under construction"
while loading the reliability page cause a hole bunch of "undefined offset" and 3 php warning of:
Cannot modify header information - headers already sent by (output started at /var/www/Loris/modules/reliability/php/reliability.class.inc:735) in /var/www/Loris/htdocs/index.php on line 48, referer: https://*instance*.loris.ca/reliability/

@driusan
Copy link
Collaborator Author

driusan commented Apr 30, 2018

@PapillonMcGill Neither the test plan nor the code for the data_team_helper module make any reference to reliability. Where are you talking about?

@PapillonMcGill
Copy link
Contributor

PapillonMcGill commented Apr 30, 2018

@driusan
For reliability, path is: clinical -> reliability.
on the page itself, the data table is empty with an "error loading data" box, the warning are from apache log.
tested with afb9099 version

@driusan
Copy link
Collaborator Author

driusan commented Apr 30, 2018

@PapillonMcGill I don't think it's worth spending a lot of time on reliability, because it's a one project module that just can't be removed from LORIS because it can't be fully converted to a standalone module (yet, due to dependencies on NDB_Caller), but I managed to get one reliability instrument to load with the latest commit, it just falls back on main.php (like instruments for now) until it's properly converted.

return true;
} else {
$Factory = NDB_Factory::singleton();
$DB = $Factory->Database();
Copy link
Contributor

Choose a reason for hiding this comment

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

the database function is lowercase

xlecours
xlecours previously approved these changes May 7, 2018
Copy link
Contributor

@xlecours xlecours left a comment

Choose a reason for hiding this comment

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

Feels like a leap of faith but I know it should work. And The Travis do not complain.

@driusan
Copy link
Collaborator Author

driusan commented May 7, 2018

@xlecours Can you re-approve? No changes, but I merged aces/major to fix the conflicts..

@xlecours
Copy link
Contributor

xlecours commented May 8, 2018

@driusan, I will wait for the Travis fixes to get in.

@driusan driusan merged commit 8d1f30f into aces:major May 9, 2018
@ridz1208 ridz1208 added this to the 20.0 milestone May 14, 2018
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.

4 participants