Skip to content

Removing ExternalModules static calls #98

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

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

jrpence
Copy link

@jrpence jrpence commented Aug 11, 2020

Changing from ExternalModules call to get project settings from the framework call

@jrpence jrpence requested a review from ChemiKyle August 11, 2020 21:30
@ChemiKyle
Copy link
Contributor

ChemiKyle commented Aug 20, 2020

Referencing the ExternalModules class directly is actually the faux pas, not simply the static call. At a glance, this one seems like an easy correction though.

Since this module doesn't specify a framework_version in the config.json, it may be unwise to reference framework, as that borrows from whatever framework version ships with the version of REDCap this module is used on.


$q = ExternalModules::getSettings('send_rx', $project_id);
if (!db_num_rows($q)) {
$projSettings = $externalModule->framework->getProjectSettings($project_id);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
$projSettings = $externalModule->framework->getProjectSettings($project_id);
$projSettings = $externalModule->getProjectSettings($project_id);

Enabling the module fails if using ->framework:

REDCap crashed due to an unexpected PHP fatal error!
Error message: Uncaught Error: Call to a member function getProjectSettings() on null in /var/www/html/modules/send_rx_v0.0.0/includes/send_rx_functions.php:40

Removing ->framework makes it function properly. Likely that we don't have access to framework because the module does not specify a framework_version.

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.

2 participants