ILIAS icon indicating copy to clipboard operation
ILIAS copied to clipboard

Add ext-pdo as required extension in composer.json

Open PurHur opened this issue 1 year ago • 3 comments

Just a small addition because i skipped over this extension and wondered about the random error messages way toooo long :(

PurHur avatar Jun 04 '24 07:06 PurHur

I assign this to you @chfsx as responsible for the Database component. I hope this is ok with you.

Best, @kergomard

kergomard avatar Jun 11 '24 13:06 kergomard

I'll also include @klees in this discussion, as he can certainly say something about this on the setup side.

Thank you very much for the PR! in principle, I agree that we should check or specify this at some point. But I think we need to bear the following in mind:

the composer.json is only relevant during the "build" process, so I think we should only have extensions here that are needed during the build process, not those that are needed for the operation of ILIAS. Imagine that the build is made on a different "machine" than ILIAS is then run productively, so this distinction is important. Extensions required for operation should rather be named in the respective components as \ILIAS\Setup\Condition\PHPExtensionLoadedCondition, so in this case here in ilDatabaseSetupAgent.

But there are certainly other php extensions that are not checked anywhere. A few are already listed here:

class.ilSetupAgent.php

but that's not all of them. would you have time to change the PR so that all required php extensions are implemented as PHPExtensionLoadedCondition in the components that mainly require the extension? it would be about the list from https://github.com/ILIAS-eLearning/ILIAS/blob/trunk/docs/configuration/install.md#php-installation-and-configuration. I honestly don't know how we want to deal with the optional extensions either: https://github.com/ILIAS-eLearning/ILIAS/blob/trunk/docs/configuration/install.md#optional-dependencies

chfsx avatar Jun 12 '24 04:06 chfsx

Hi @PurHur, hi @chfsx,

I think we could very well accept this PR and make the other changes in separate iterations. In general I think @chfsx is correct regarding machines for build and deployment, so this won't prevent any problems there. Still, having different machines for build and deployment is a somewhat elaborate setup where people need to check environments anyway, e.g. "is the correct version of PHP installed". We won't be able to save everyone there. So I think this is a good additional way to document requirements.

Regarding the checks during the setup: I would much appreciate if someone added checks for PHP extensions, as @chfsx mentions. From my POV they would best be added in the components that actually require the extensions. We could then treat the PHP extensions just like any other dependency and discuss them (briefly) every year. Optional dependencies could be checked in the component as well. The according checks could be made conditional on some config, so it should be possible to run checks only if extensions are needed, if there is according config.

Kind regards!

klees avatar Jun 12 '24 06:06 klees

Thank you all for your opinion on this,

we had a brief internal discussion and came to an agreement. @thojou will provide a PR for this and similar extensions which then will pass the JF for some brief approval. Through this process we hope to integrate the majority of "trivial" extensions to the composer and may add more "critical" extension through the following iterations.

Therefore, I will close this PR.

Greetings, @iszmais

iszmais avatar Feb 26 '25 11:02 iszmais