Skip to content
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

Modernize the codebase #85

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

Modernize the codebase #85

wants to merge 11 commits into from

Conversation

GCth
Copy link

@GCth GCth commented Oct 31, 2024

A lot of changes, so I expect merge is unlikely, but I'd like to throw my 2 cents here.

STDERR replaced with proper PSR logger, errors with exceptions, and lot of code cleanup using modern static analysis and code style tools.

There is still some work to be done to make the code fully typed, but my intention was to accommodate this code to be used with modern Symfony apps as a library. As for future tasks, removing the false return type would also make the code cleaner, so would be a separation between service layer and data layer. Reducing the functions complexity would be nice too.
I'd love to have some unit tests here, as I'm not an expert on PDF. Manual tests with the signing functionality I needed are working as previously.

@dealfonso
Copy link
Owner

Hi,

while I appreciate your effort, my intention is not to merge this PR. There are multiple reasons (e.g.):

  • At this moment, there is no need to drop the support for PHP 7. Maybe I can create a branch for that, but I think that there are people using it.
  • You are including files very specific to phpstan... which I think that should not be a dependency
  • There are spaces, parenthesis, comments, etc. that, while not mandatory, are left intentionally
  • Others (e.g.:) at this moment I prefer using p_error like functions than exceptions because it is possible to show errors, but are recoverable (i.e. the code may continue).

I'll leave this PR opened by now to gather the opinion of others and maybe reconsidering to merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants