Skip to content

[FEATURE] UI: Implement initialisation using new component mechanism#7969

Merged
thibsy merged 2 commits intoILIAS-eLearning:trunkfrom
srsolutionsag:feature/10/ui-initialisation
Dec 11, 2024
Merged

[FEATURE] UI: Implement initialisation using new component mechanism#7969
thibsy merged 2 commits intoILIAS-eLearning:trunkfrom
srsolutionsag:feature/10/ui-initialisation

Conversation

@thibsy
Copy link
Contributor

@thibsy thibsy commented Aug 20, 2024

Hi @klees,

I have finished my work on the new UI framework initialisation!

In order to incorporate the UI framework initialisation, which uses the new component bootstrap mechanism, I needed to create some sort of bridge between these components and the legacy initialisation.

Let me summarise the relevant changes of this PR:

  • Introduction of Init\AllModernComponents: In order to incorporate bootstrapped components into the legacy initialisation, or to create some kind of bridge between those two things, I had to introduce a new Component\EntryPoint which should be used instead of a direct call to ilInitialisation::initILIAS(). This class basically maps bootstrapped components and populates them inside the legacy service locator $DIC with the same offsets as before. This is rather important, because our DI\Container and also lots of array-access-like usages will be made to these offsets throughout the code base. Btw, I think this adapter beautifully shows how ugly a service locator really is.
  • Language\Language::loadLanguageModule() calls: I needed to move some of these method calls into other methods, because they were performed in the constructor. This is a bad idea in general, but in this specific scenario it led to issues, since functionality was required inside the bootstrap build process of components which have not been migrated and was therefore not available.
  • Introduction of Language\LanguageLegacyInitialisationAdapter and UICore\GlobalTemplateInitialisationAdapter: I needed to add some "legacy component" adapters in order to inject some implementation into bootstrapped components, of components which are not yet migrated. This needed to be done so functionality is kept alive at runtime, while keeping things compatible with the bootstrap build process.
  • Bugfix for ILIAS\LegalDocuments\Setup\ConsumerObjective: I noticed that this objective was flaky, because it depended on the class.ilInitialisation.php file to be loaded first. This file contained the initialisation of the global $DIC outside of the class declaration, so loading this file to check for interface implementations declared this variable as a side effect. (ping @mjansenDatabay)
  • UI framework plugin manipulations: The UI initialisation exposes all of its component factories inside $provide, in order to support the legacy mechanism which lets plugins exchange them. We have already discussed that we should not support this mechanism in the future, but until then we needed to expose them.
  • Removal of UI\Implementation\Renderer\Template::addInLoadCode(): I noticed this method had no usages, so I removed it to get rid of the ilGlobalPageTempalte dependency there and hereby in UI\Implementation\Renderer\ilTemplateWrapperFactory too.
  • Extension of Language\Language interface: I noticed the UI framework accesses methods of this service which have not been populated on the interface yet, I therefore added them. (ping @katringross)
  • Introduction of UICore\GlobalTemplate interface: I added a UICore\GlobalTemplate interface which we can use to type-hint the (legacy) ilGlobalPageTemplate like we do with language.
  • Slight usability improvement: I changed the static strings inside the dependency_resolution.php files to use the ::class constant of components, so renaming things will also affect these disambiguation files. (stumbled over this two, three times)

Kind regards,
@thibsy

@thibsy thibsy self-assigned this Aug 20, 2024
@thibsy thibsy force-pushed the feature/10/ui-initialisation branch from 78d4d3f to 27d9233 Compare August 20, 2024 09:49
@thibsy thibsy force-pushed the feature/10/ui-initialisation branch from 27d9233 to 1811305 Compare September 19, 2024 12:56
@thibsy thibsy added kitchen sink php Pull requests that update Php code labels Sep 19, 2024
@thibsy thibsy force-pushed the feature/10/ui-initialisation branch from 1811305 to c1d0a1c Compare September 20, 2024 06:30
@thibsy
Copy link
Contributor Author

thibsy commented Sep 20, 2024

Please note the pipelines do not yet pass, since I need to amend some of the php unit tests. There are unit tests which initialise the UI framework entirely, which is not the best idea in general. If we decide to move forward with this PR, I will rewrite said unit tests and see that the pipelines pass.

There are also 77(+) usages of ilInitialisation::initILIAS() left, which probably need to be replaced by the new entry point. Again, I would amend this if we plan to continue with this approach here.

@thibsy thibsy force-pushed the feature/10/ui-initialisation branch from c1d0a1c to 2e93f2a Compare September 23, 2024 13:03
@matthiaskunkel
Copy link
Member

Jour Fixe, 30 SEP 2024: Thibeau notified us about an upcoming workshop about the changes coming with the component revision. It takes place at 07 October, 13:00 to 14:30.

@thibsy thibsy force-pushed the feature/10/ui-initialisation branch from 2e93f2a to 3db20b1 Compare October 4, 2024 09:57
@thibsy
Copy link
Contributor Author

thibsy commented Oct 7, 2024

The workshop has been rescheduled to 21. October from 13:00 to 14:30.

@Amstutz Amstutz removed their assignment Oct 11, 2024
@thibsy thibsy force-pushed the feature/10/ui-initialisation branch from 3db20b1 to 14198b4 Compare October 29, 2024 15:22
@thibsy
Copy link
Contributor Author

thibsy commented Oct 29, 2024

I have rebased onto the latest trunk and incorporated the new prompt and progress component families.

@katringross
Copy link
Contributor

@thibsy thanks for the changes in the language interface

@matthiaskunkel
Copy link
Member

Jour Fixe, 11 NOV 2024: @thibsy asks all developers to have a final look on this PR as it should be merged next week. Please notify Thibeau if you object against the current suggestion.

@schmitz-ilias
Copy link
Contributor

Hi @thibsy, thanks for the PR, I'm sure it will be very useful as a future reference for implementing the new dependency mechanism in other components.

I'm not sure I understand the changes in Session.php. They seem to be related to KS tables, what does that have to do with the Session repository object?

@thibsy
Copy link
Contributor Author

thibsy commented Nov 12, 2024

Hi @schmitz-ilias,

Thanks for looking into the PR. It looks like I have made a mistake here. I thought ilSession belongs to the Session component. I will move the implementation to the Authentication component, where this object is actually implemented.

As to why this implementation is located outside of the framework: The component bootstrap mechanism provides a scheme of integration strategies, which e.g. let components define an interface of some service that must be implemented by another component. This is the case for the UI\Component\Table\Storage, which is currently a session-storage (see #6701) and should be implemented using ilSession. Therefore, the owner of this is required to provide an implementation of the UI framework's interface definition.

Kind regards,
@thibsy

@thibsy thibsy force-pushed the feature/10/ui-initialisation branch from 14198b4 to f57768b Compare November 12, 2024 10:55
Copy link
Contributor

@mjansenDatabay mjansenDatabay left a comment

Choose a reason for hiding this comment

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

Hi @thibsy,

thanks for the PR.

The changes in ...

  • DataProtection
  • TermsOfService

... look good to me.

Refinery:

  • Please extract $this->language->loadLanguageModule('validation'); to a method (e.g.) loadLanguageModules and move the loading of the "validation" language module to this method.

Authentication:

Please answer:

  • Why does the "Authentication" component define a session-based key/value storage used by the UI framework? Because ilSession is a file in the "Authentication" component? Somehow this looks not right for me. This is not a hard "no" or "rejected", but when doing some quick research in component/bundle-based PHP frameworks, such implementations/strategies of a "Storage"-like interface are often located within the actual component (of which "Domain" ever, here "UI"), or put in into a separate "KeyValueStorage" component. Of course ilSession is a k/v storage itself and a static class, so I could live with the proposed solution in this PR. But a "KeyValueStorage" folder (either as a separate component or within another component) would be my personal preference.

Best regards,
Michael

@thibsy thibsy force-pushed the feature/10/ui-initialisation branch from f57768b to 7c6b7fe Compare November 12, 2024 13:37
@thibsy
Copy link
Contributor Author

thibsy commented Nov 12, 2024

Hi @mjansenDatabay,

Thanks for looking into this PR. I implemented the requested changes inside the Refinery\Factory. To answer your question about the location of the session-storage implementation:

I agree that Authentication is not the right place to implement the key-value storage needed by the UI framework. I also agree that it would be nice to have a more sophisticated solution for storages like this, ideally with a common interface. But for this PR it seems to be out of scope and we should definitely put some more thought into this. Therefore, I keep the implementation there with the same comment, mentioning a proper service to be implemented.

Kind regards,
@thibsy

@thibsy thibsy force-pushed the feature/10/ui-initialisation branch 3 times, most recently from 757ced0 to 499208c Compare December 2, 2024 14:48
@thibsy thibsy force-pushed the feature/10/ui-initialisation branch from 499208c to a59b171 Compare December 11, 2024 10:49
@thibsy thibsy merged commit d6b3b32 into ILIAS-eLearning:trunk Dec 11, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

improvement kitchen sink php Pull requests that update Php code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants