Skip to content

refactor: migrate controllers away from ValidateHelper - #8383

Merged
vitormattos merged 2 commits into
LibreSign:mainfrom
vikas-kushwaha-dev:refactor/8353-migrate-validate-helper
Sep 15, 2026
Merged

vitormattos merged 2 commits into
LibreSign:mainfrom
vikas-kushwaha-dev:refactor/8353-migrate-validate-helper

Conversation

@vikas-kushwaha-dev

Copy link
Copy Markdown
Contributor

Resolves: #8353

📝 Summary

Migrates the remaining simple controller consumers away from ValidateHelper and uses the focused validators directly.

  • AccountController: removes the unused ValidateHelper dependency and import.
  • FileController: replaces ValidateHelper with FileInputValidator for file validation and SigningRequestValidator for signing-request validation.
  • PageController: replaces ValidateHelper with SigningRequestValidator and updates PageControllerTest accordingly.

This keeps the existing validation and signing-request behaviour unchanged while moving these controllers towards the dedicated validator classes.

No public API or UI behaviour is changed by this PR.

🧪 How to test

Run the affected controller tests:

php vendor/bin/phpunit -c tests/php/phpunit.xml --no-coverage tests/php/Unit/Controller/PageControllerTest.php
php vendor/bin/phpunit -c tests/php/phpunit.xml --no-coverage tests/php/Api/Controller/FileControllerTest.php
php vendor/bin/phpunit -c tests/php/phpunit.xml --no-coverage tests/php/Api/Controller/AccountControllerTest.php

The affected controller tests pass when run individually.

The complete unit test suite was also run locally. It completed all 3,860 tests, but the local devcontainer reported unrelated failures, primarily caused by Nextcloud appdata permission issues and API test state/authentication issues unrelated to this refactor.

Additional check:

git diff --check

passes without errors.

⚙️ API / Back-end changes

  • Removed the unused ValidateHelper dependency from AccountController.
  • Migrated FileController to focused validators.
  • Migrated PageController to SigningRequestValidator.
  • Updated the affected PageControllerTest.
  • Existing signing-request behaviour preserved.
  • No public API changes.
  • No OpenAPI update required.

🚧 Backport

None (main only).

✅ Checklist

🤖 AI (if applicable)

  • The content of this PR was partially or fully generated using AI.

@welcome

welcome Bot commented Sep 13, 2026

Copy link
Copy Markdown

Thanks for opening your first pull request in this repository! ✌️

@vitormattos

Copy link
Copy Markdown
Member

@vitormattos vitormattos left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The php-cs check confirms that the new validator imports need to be reordered.

composer run cs:check reports the same issue in three files:

  • lib/Controller/FileController.php
  • lib/Controller/PageController.php
  • tests/php/Unit/Controller/PageControllerTest.php

Could you please run composer run cs:fix and commit the resulting import ordering changes?

@github-project-automation github-project-automation Bot moved this from 0. Backlog to 1. to do in Roadmap Sep 13, 2026
@vikas-kushwaha-dev
vikas-kushwaha-dev force-pushed the refactor/8353-migrate-validate-helper branch from 2f5a745 to 0abb92d Compare September 14, 2026 10:08

@vitormattos vitormattos left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The code changes look good now. The import ordering issue is fixed.

There are two things left before this PR can be merged: the branch has a conflict with main, and the DCO check shows that the first commit is missing the required sign-off.

If you do not already have the LibreSign repository configured as upstream, you can add it with:

git remote add upstream https://github.com/LibreSign/libresign.git
git fetch upstream

Then rebase your branch on the current main and add the DCO sign-off to the commits:

git rebase --signoff upstream/main

If Git reports a conflict, resolve the conflicting files, then run:

git add <resolved-files>
git rebase --continue

Repeat this until the rebase finishes.

After that, update your PR branch with:

git push --force-with-lease origin refactor/8353-migrate-validate-helper

Please use --force-with-lease instead of --force, as it avoids overwriting unexpected remote changes.

LibreSign requires DCO sign-off on every commit. DCO (Developer Certificate of Origin) is a lightweight way to certify that you have the right to contribute the code under the project's license. Git records this with a line like:

Signed-off-by: Your Name <your@email>

Using git commit -s when creating commits adds this automatically. In this PR, your second commit already has the sign-off; the DCO check is reporting the first commit.

We also have this documented here:
https://github.com/LibreSign/documentation/blob/main/developer_manual/getting-started/commits.rst

More about DCO:
https://developercertificate.org/

After the rebase and push, GitHub should recalculate the conflict status and the DCO check.

@vikas-kushwaha-dev
vikas-kushwaha-dev force-pushed the refactor/8353-migrate-validate-helper branch from 0abb92d to 59ce952 Compare September 14, 2026 19:41
@vikas-kushwaha-dev

Copy link
Copy Markdown
Contributor Author

Hi @vitormattos, I’ve addressed the requested changes. The branch has been rebased, the commit history has been cleaned up, and the DCO check is now passing. Could you please re-review the PR and approve the pending workflows when you have a chance? Thanks!

Signed-off-by: vikas-kushwaha-dev <vikaskushwaha.dev00@gmail.com>
Signed-off-by: vikas-kushwaha-dev <vikaskushwaha.dev00@gmail.com>
@vitormattos
vitormattos force-pushed the refactor/8353-migrate-validate-helper branch from 59ce952 to 3964329 Compare September 15, 2026 02:19
@vitormattos vitormattos added this to the Next Major (36) milestone Sep 15, 2026
@vitormattos
vitormattos merged commit 59afb34 into LibreSign:main Sep 15, 2026
67 checks passed
@github-project-automation github-project-automation Bot moved this from 1. to do to 4. to release in Roadmap Sep 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: 4. to release

Development

Successfully merging this pull request may close these issues.

Migrate remaining controller consumers away from ValidateHelper

2 participants