-
Notifications
You must be signed in to change notification settings - Fork 3
Added support for os2web_key #101
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: develop
Are you sure you want to change the base?
Conversation
1141148
to
fc90fbe
Compare
5ae3ae7
to
b274156
Compare
37f77e4
to
d82195e
Compare
… to get certificate. # Conflicts: # CHANGELOG.md # composer.json # modules/os2forms_digital_post/src/Helper/CertificateLocatorHelper.php # modules/os2forms_digital_post/src/Helper/DigitalPostHelper.php
d82195e
to
801cae9
Compare
6d9f63c
to
a8dc3c8
Compare
be0d2cc
to
fea276d
Compare
…per.php Co-authored-by: Mikkel Ricky <[email protected]>
Updated digital post handler
c4fd652
to
6ba43e9
Compare
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.
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.
.github/workflows/pr.yml
Outdated
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.
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.
.gitignore
Outdated
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.
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.
.markdownlint.jsonc
Outdated
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.
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.
.markdownlintrc
Outdated
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.
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.
CHANGELOG.md
Outdated
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.
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.
package.json
Outdated
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.
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.
scripts/code-analysis
Outdated
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.
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.
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.
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.
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.
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.
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.
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
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. |
*/ | ||
function os2forms_fasit_update_9001(): void { | ||
\Drupal::service('module_installer')->install([ | ||
'key', |
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 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
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 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.
idea was to remove refactoring to another PR (https://github.com/OS2Forms/os2forms/pull/168/files). I see it was done, but this is still marked as change for this PR
can you comment what is the idea here? Thanks
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.
@stankut, as mentioned under “Caution” in the pull request description, the branch for this pull request has feature/code-cleanup as base (to benefit from the code cleanup).
Therefore we (you or I) should merge #168 to get a more clean view on what's actually changed on in this pull request. I don't remember if I'm allowed to merge my own pull requests when approved or if you have to do it.
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 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.
README.md
Outdated
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.
idea was to remove refactoring to another PR (https://github.com/OS2Forms/os2forms/pull/168/files). I see it was done, but this is still marked as change for this PR
can you comment what is the idea here? Thanks
CHANGELOG.md
Outdated
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.
idea was to remove refactoring to another PR (https://github.com/OS2Forms/os2forms/pull/168/files). I see it was done, but this is still marked as change for this PR
can you comment what is the idea here? Thanks
.markdownlintignore
Outdated
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.
idea was to remove refactoring to another PR (https://github.com/OS2Forms/os2forms/pull/168/files). I see it was done, but this is still marked as change for this PR
can you comment what is the idea here? Thanks
.github/workflows/pr.yml
Outdated
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.
idea was to remove refactoring to another PR (https://github.com/OS2Forms/os2forms/pull/168/files). I see it was done, but this is still marked as change for this PR
can you comment what is the idea here? Thanks
@@ -58,7 +58,8 @@ | |||
"itk-dev/serviceplatformen": "^1.5", | |||
"mglaman/composer-drupal-lenient": "^1.0", | |||
"os2web/os2web_audit": "^1.0", | |||
"os2web/os2web_datalookup": "^2.0", | |||
"os2web/os2web_datalookup": "dev-feature/os2web_key as 2.1.0", |
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.
shall we maybe merge os2web_datalookup before merging this PR?
then extra repository mention will not be needed
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.
Yes, we should do that. This PR depends on many other PRs. I've requested a review from you on OS2web/os2web_datalookup#13.
# Conflicts: # CHANGELOG.md
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.