-
-
Notifications
You must be signed in to change notification settings - Fork 405
Moodle: Prepare for an upcoming change in Moodle distribution #551
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
base: main
Are you sure you want to change the base?
Conversation
0ce4049 to
476e310
Compare
|
Hi @Seldaek, I just wanted to check if we're still ok to go for this approach (submitting patches for the Composer installer), or would you recommend we roll our own plugin which is specific to Moodle? I note that the recommendation in Composer docs is to use the new Composer API to access plugins by type from the vendor directory, but sadly we're some time away from being able to do this (though it is on my radar). I also note that there is a related PR (#550) which has been waiting for PR for some time and this project has not seen a patch for some time. Cheers |
476e310 to
b1a2451
Compare
This set of changes adds support for: - the 'public' directory introduced in Moodle 5.1 - the ability for Moodle to be installed as a Composer dependency - configuration of an installation-specific prefix for the Moodle 'core' path I've also: - updated the plugin type list for Moodle 5.1 - removed the `admin_report` plugin type which has not been a valid plugin type for about 15 years.
b1a2451 to
3fbe06d
Compare
|
Hey @andrewnicols I think it's fine to update the installer here if you're ok with slight delays.. I'll try to get on this review at some point but might be another couple weeks. |
| ); | ||
| $extra = $moodlePackage ? $moodlePackage->getExtra() : $this->composer->getPackage()->getExtra(); | ||
|
|
||
| return !empty($extra['haspublicdir']) ? 'public/' : ''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not has-public-dir?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah never mind I see this reads from moodle itself and not just the root package so you cannot exactly fix this name anymore I guess.
| { | ||
| // The public directory setting is stored in the main Moodle package's. | ||
| // Legacy Moodle installs do not have this path, or any setting. | ||
| $moodlePackage = $this->composer->getRepositoryManager()->findPackage( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this would be safer but please do check it works for your use case still.
| $moodlePackage = $this->composer->getRepositoryManager()->findPackage( | |
| $moodlePackage = $this->composer->getRepositoryManager()->getLocalRepository()->findPackage( |
| string $composerType, | ||
| array $rootExtras, | ||
| array $moodleExtras, | ||
| string $packageName, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| string $packageName, | |
| string $packageName |
Hi,
I am the Principal Architect for Moodle and I'm tryign to move Moodle to officially support installation via Composer.
This set of changes adds support for:
I've also:
admin_reportplugin type which has not been a valid plugin type for about 15 years.