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

Remove the externallib dependency #105

Open
wants to merge 1 commit into
base: MOODLE_33PLUS
Choose a base branch
from

Conversation

wo14580
Copy link
Contributor

@wo14580 wo14580 commented Apr 17, 2024

Since the version of Moodle 4.2, the unit tests were broken because of the lib/externallib.php. It imposed to isolate the unit tests.
Instead of adding those following annotations as mentioned:

  • @runInSeparateProcess
  • @runTestsInSeparateProcesses.

I preferred removing the lib/externallib.php dependency.This library will be deprecated soon based on this ticket:
https://tracker.moodle.org/browse/MDL-76583.

Now the unit tests are working correctly.
And we did some manuals tests in our environment and it works correctly.

Cette version de code n'est pas compatible avec les versions avant Moodle 4.2

@dmitriim
Copy link
Member

Hi @wo14580

Thanks for looking at this. Your code looks good. However, it seems like we can try to apply a bit more solid approach:

  1. Get rid of externallib.php completely and refactor it out to a namespaced class. Example https://github.com/catalyst/moodle-tool_dynamic_cohorts/blob/MOODLE_401_STABLE/classes/external/rule_conditions.php
  2. Having (1) we need to update https://github.com/catalyst/moodle-auth_userkey/blob/MOODLE_33PLUS/db/services.php and point auth_userkey_request_login_url to a new class.
  3. Move externallib_test.php to match a new class localtion and name space.
  4. Update https://github.com/catalyst/moodle-auth_userkey/blob/MOODLE_33PLUS/version.php#L32 so CI runs tests against Moodle 4.2
  5. Bump a plugin version.

Unfortunately, we won't be able to remove require_once($CFG->dirroot . '/lib/externallib.php'); at this stage as it's required for older versions, unless we want to spin a new branch. But as long as we can easily keep the branch backwards compatible, I'd prefer to keep it all in one branch. (We will remove this dependency as soon as we get a new branch out. Like 4.4+ or something.)

Hopefully it's not too many changes and thanks again for your contribution. Really appreciate it.

@wo14580
Copy link
Contributor Author

wo14580 commented Apr 24, 2024

Hi @dmitriim,
Thank you for your feedback and analyse.
I didn't have time to go deeper when I worked on it but I agree with you.

I'll try to work on these remarks when I have some free time.

Do you have an action plan for future versions in order to be aligned with the new versions of Moodle ?

Thank you and have a nice day.

@dmitriim
Copy link
Member

dmitriim commented Apr 28, 2024

Hi @wo14580
I don't think we have clients using this plugin who's moving to the latest version any time soon. So at this stage it all depends on a community. I'm happy to code review and etc.

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