-
Notifications
You must be signed in to change notification settings - Fork 10
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
ENH Update reference to supported modules data #220
ENH Update reference to supported modules data #220
Conversation
@@ -14,9 +14,12 @@ class SupportedAddonsLoader extends ApiLoader | |||
*/ | |||
public function getAddonNames() | |||
{ | |||
$endpoint = 'https://raw.githubusercontent.com/silverstripe/supported-modules/5/modules.json'; | |||
$endpoint = 'https://raw.githubusercontent.com/silverstripe/supported-modules/main/repositories.json'; |
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.
Look at changing $this->doRequest()
to the new method in supported-modules
If non-trivial then don't bother
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.
This class is only used in one place, and the level of caching done by ApiLoader
for this is unnecessary.
I'm deprecating this class and just updating BringYourOwnIdeas\Maintenance\Tasks\UpdatePackageInfoTask::getSupportedPackages()
to use the PHP API instead.
I'll leave this implementation as it is now in case someone is calling this method for their own nefarious purposes.
78d046e
to
ec09587
Compare
src/Tasks/UpdatePackageInfoTask.php
Outdated
return $this->getSupportedAddonsLoader()->getAddonNames() ?: []; | ||
$repos = MetaData::getAllRepositoryMetaData()[MetaData::CATEGORY_SUPPORTED]; | ||
$version = VersionProvider::singleton()->getModuleVersion('silverstripe/framework'); | ||
preg_match('/^([0-9]+)/', $version, $matches); | ||
$cmsMajor = BranchLogic::getCmsMajor(MetaData::getMetaDataForRepository('silverstripe/silverstripe-framework'), $matches[1] ?? ''); | ||
return array_filter(array_map( | ||
fn(array $item) => isset($item['majorVersionMapping'][$cmsMajor]) ? $item['packagist'] : null, | ||
$repos | ||
)); |
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.
Note this is not hardcoded to 5
anymore, to reduce the changes of it being stale if this module gets a CMS 6 compatible release.
return $this->doRequest($endpoint, function ($responseJson) { | ||
return array_map(fn(array $item) => $item['composer'], $responseJson); | ||
return array_filter(array_map( | ||
fn(array $item) => isset($item['majorVersionMapping'][5]) ? $item['packagist'] : null, |
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.
Didn't bother making the cms major here dynamic since this is deprecated anyway.
ec09587
to
afe86ba
Compare
afe86ba
to
85be422
Compare
public function testGetSupportedPackagesEchosErrors() | ||
{ | ||
$supportedAddonsLoader = $this->mockSupportedAddonsLoader(); | ||
$moduleHealthLoader = $this->mockModuleHealthLoader(); | ||
|
||
$supportedAddonsLoader->expects($this->once()) | ||
->method('getAddonNames') | ||
->will($this->throwException(new RuntimeException('A test message'))); | ||
|
||
/** @var UpdatePackageInfoTask $task */ | ||
$task = UpdatePackageInfoTask::create(); | ||
$task->setSupportedAddonsLoader($supportedAddonsLoader); | ||
$task->setModuleHealthLoader($moduleHealthLoader); | ||
|
||
ob_start(); | ||
$task->getSupportedPackages(); | ||
$output = ob_get_clean(); | ||
|
||
$this->assertStringContainsString('A test message', $output); | ||
} |
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.
There's not a clean way to mock this anymore - but it's also very clear and deliberate code that causes this to happen, we'd have to intentionally change it to not echo the exception in order to introduce a regression here.
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.
Tested locally, seems to work
Has two commits - the main purpose is to pull in data from the new
main
branch of silverstripe/supported-modules, but #218 had to be fixed at the same time or the task throws exceptions.Issues