Added support for os2web_key #101

rimi-itk · 2024-04-26T13:51:07Z

Adds support for os2web_key

Caution

The feature/os2web_key branch in this pull request is branched from feature/code-cleanup and therefore the pull request Code clean-up #168 should be addressed – and merged – before code reviewing this pull request.

modules/os2forms_digital_post/src/Form/SettingsForm.php

… to get certificate. # Conflicts: # CHANGELOG.md # composer.json # modules/os2forms_digital_post/src/Helper/CertificateLocatorHelper.php # modules/os2forms_digital_post/src/Helper/DigitalPostHelper.php

Updated digital post handler

stankut

Hi!

Most of my comments here are about how the changes are grouped. I see there have been a lot of refactoring changes—this is superb! However, these are currently mixed with new functionality, such as the addition of the os2web_key module, which makes the overall changeset look cluttered.

My suggestion is to try to keep different types of changes separate—perhaps by grouping them into two or three additional PRs.

stankut · 2025-04-28T07:53:13Z

.github/workflows/pr.yml

We are updating/refactoring github workflow.
In my opinion it would clearer to make it as part of the separate/dedicated PR, not to mix it with new functionality.

stankut · 2025-04-28T07:53:43Z

.gitignore

We are updating/refactoring general files in repository.
In my opinion it would clearer to make it as part of the separate/dedicated PR, not to mix it with new functionality.

stankut · 2025-04-28T07:53:49Z

.markdownlint.jsonc

We are updating/refactoring general files in repository.
In my opinion it would clearer to make it as part of the separate/dedicated PR, not to mix it with new functionality.

stankut · 2025-04-28T07:53:52Z

.markdownlintrc

We are updating/refactoring general files in repository.
In my opinion it would clearer to make it as part of the separate/dedicated PR, not to mix it with new functionality.

stankut · 2025-04-28T07:55:18Z

CHANGELOG.md

if we are to keep the refactoring of the file not related with the new functionality (os2web_key), that must be reflected in CHANGELOG.
my suggestion is to make that as part of separate PR.

stankut · 2025-04-28T09:48:53Z

package.json

We are updating/refactoring other files in repository (not related with os2web_key)
In my opinion it would clearer to make it as part of the separate/dedicated PR, not to mix it with new functionality.

stankut · 2025-04-28T09:48:59Z

scripts/code-analysis

We are updating/refactoring other files in repository (not related with os2web_key)
In my opinion it would clearer to make it as part of the separate/dedicated PR, not to mix it with new functionality.

stankut · 2025-04-28T09:49:42Z

modules/os2forms_digital_post/src/Helper/Settings.php

We are updating/refactoring other files in repository (not related with os2web_key)
In my opinion it would clearer to make it as part of the separate/dedicated PR, not to mix it with new functionality.

stankut · 2025-04-28T09:51:14Z

modules/os2forms_fasit/src/Form/SettingsForm.php

We are updating/refactoring other files in repository (not related with os2web_key)
In my opinion it would clearer to make it as part of the separate/dedicated PR, not to mix it with new functionality.

stankut · 2025-04-28T09:52:06Z

modules/os2forms_fasit/src/Helper/Settings.php

We are updating/refactoring other files in repository (not related with os2web_key)
In my opinion it would clearer to make it as part of the separate/dedicated PR, not to mix it with new functionality.

# Conflicts: # .github/workflows/pr.yml # CHANGELOG.md # README.md # composer.json # modules/os2forms_dawa/src/Plugin/os2web/DataLookup/DatafordelerDataLookup.php # modules/os2forms_digital_post/src/Drush/Commands/DigitalPostTestCommands.php

rimi-itk · 2025-04-30T10:17:18Z

Hi!

Most of my comments here are about how the changes are grouped. I see there have been a lot of refactoring changes—this is superb! However, these are currently mixed with new functionality, such as the addition of the os2web_key module, which makes the overall changeset look cluttered.

My suggestion is to try to keep different types of changes separate—perhaps by grouping them into two or three additional PRs.

I totally agree, @stankut, and I cannot remember why I mixed everything together. I've moved the code cleanup stuff to a new pull request, #168.

stankut · 2025-05-28T11:38:28Z

modules/os2forms_fasit/os2forms_fasit.install

+ */
+function os2forms_fasit_update_9001(): void {
+  \Drupal::service('module_installer')->install([
+    'key',


I did not find drupal/key to be present in this module composer nor in info.yml files.
it is however a dependency for os2web_key but that is too much of a uncertainty.

So here we have no guarantee that this module even exist in our installation. That can potentially lead to a situation where we are trying to enable a module we did not request.

suggestion: require it via composer/info.yml file OR remove the enabling via hook

Nice catch, @stankut. I've updated the dependency in 2ecaa82. The os2forms_fasit module does not have its own composer.json file, but relies on the one from the parent module os2forms.

stankut · 2025-05-28T11:39:48Z

composer.json