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

[LorisInstance] add getModule function #8221

Merged
merged 3 commits into from
Dec 19, 2022
Merged

Conversation

driusan
Copy link
Collaborator

@driusan driusan commented Nov 7, 2022

LorisInstance already has functions to check if a module exists, get the module directories, etc.

This adds a getModule to replace \Module::factory() so that the code dealing with modules can be centralized in one place.

Comment on lines 79 to 80
$mod = \Module::factory($this, $name);
$modules[] = $mod;
Copy link
Collaborator

Choose a reason for hiding this comment

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

shouldnt this use getModule() ??

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not sure why this was committed, I think I was just trying to var_dump what got added to debug an exception since the new code is semantically equivalent to the old.. but sure, I can change it to getModule.

Copy link
Collaborator

@ridz1208 ridz1208 left a comment

Choose a reason for hiding this comment

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

are you planning on replacing the existing instances of module::factory? otherwise it will never be replaced, we would just be adding another way of doing it

@driusan
Copy link
Collaborator Author

driusan commented Nov 16, 2022

Yes, I'm planning on removing completely removing \Module::factory calls in a different PR. I removed the ones that are in standard LORIS endpoints in the latest commit since they all had access to a LorisInstance object.

When the code is it's moved, it'll use $loris->getModuleDirs() instead of hardcoded paths. So I left the calls in scripts or ajax endpoints which will break since moduleDirs isn't properly set right now and they just use an empty [] directory stub.

I also left the ones that are just using \Module::factory for the side-effect of registering the autoloader. I think when the loading code is moved to getModule() I'll likely decouple the side-effect of the autoloader from the getting (ie. add something like a $module->registerAutoloader() function and it'll help identify the places that need to be updated.

LorisInstance already has functions to check if a module
exists, get the module directories, etc.

This adds a getModule to replace \Module::factory() so that
the code dealing with modules can be centralized in one place.
@driusan driusan added the Blocking PR should be prioritized because it is blocking the progress of another task label Dec 19, 2022
@driusan driusan merged commit 09dbb2f into aces:main Dec 19, 2022
driusan added a commit that referenced this pull request Feb 22, 2023
…>getModule() (#8287)

This finishes what was started in #8221 and eliminates the \Module::factory
method. All remaining instances are updated to use $loris->getModule instead,
which takes that LORISInstance module paths into consideration.
@ridz1208 ridz1208 added this to the 25.0.0 milestone Mar 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Blocking PR should be prioritized because it is blocking the progress of another task
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants