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.

Sign up for GitHub

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

Jump to bottom

Add PHPStan and fix level 1 errors #307

Merged
merged 4 commits into from
Dec 12, 2022
Merged

Add PHPStan and fix level 1 errors #307

merged 4 commits into from
Dec 12, 2022

Conversation

mvorisek
Copy link
Contributor

No description provided.

@mvorisek mvorisek mentioned this pull request Mar 9, 2021
Merged
@williamdes

This comment was marked as off-topic.

Sign in to view
@robertotremonti
Copy link

@mvorisek could you open this at WarnockPDF, the fork of TCPDF ?
Some of your changes look great, some look strange

What's the meaning of your fork, @williamdes ?

@williamdes

This comment has been minimized.

Sign in to view
@mvorisek mvorisek mentioned this pull request Mar 22, 2021
Merged
@mvorisek mvorisek closed this Mar 25, 2021
@mvorisek mvorisek deleted the gh_add_phpstan branch March 25, 2021 11:20
williamdes added a commit to code-lts/WarnockPDF that referenced this pull request Mar 26, 2021
@williamdes
Merge tecnickcom/TCPDF#307 - Solve the error when try to remove a fil…
48676e3 
…e previously removed, the idea is to remove first the imagekeys files and then, the remaining files of the temporary directory

Pull-request: #6
@mvorisek mvorisek restored the gh_add_phpstan branch March 27, 2021 10:40
@mvorisek mvorisek reopened this Mar 27, 2021
nicolaasuni
nicolaasuni requested changes Mar 27, 2021
View reviewed changes
Copy link
Member

@nicolaasuni nicolaasuni left a comment

Choose a reason for hiding this comment

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

There is too much going on in this PR.
Would you mind split it in small parts.
Note that some code like the barcode shouldn't used at all as there is a new lib for that.

include/barcodes/datamatrix.php Show resolved Hide resolved
include/barcodes/pdf417.php Outdated Show resolved Hide resolved
include/barcodes/qrcode.php Show resolved Hide resolved
include/tcpdf_colors.php Outdated Show resolved Hide resolved
include/tcpdf_static.php Show resolved Hide resolved
tcpdf.php Show resolved Hide resolved
nicolaasuni
nicolaasuni previously approved these changes Apr 2, 2021
View reviewed changes
@nicolaasuni
Copy link
Member

Can you please sort out the conflicts?

williamdes
williamdes reviewed Apr 2, 2021
View reviewed changes
composer.json Outdated Show resolved Hide resolved
williamdes
williamdes suggested changes Apr 2, 2021
View reviewed changes
Copy link
Contributor

@williamdes williamdes left a comment

Choose a reason for hiding this comment

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

Could you remove the composer change ?

@codecov-io
Copy link

codecov-io commented Apr 3, 2021 •
edited
Loading

Codecov Report

Merging #307 (6b160ab) into main (c979d00) will decrease coverage by 27.08%.
The diff coverage is 81.25%.

Impacted file tree graph

@@             Coverage Diff             @@
##             main     #307       +/-   ##
===========================================
- Coverage   72.80%   45.71%   -27.09%     
===========================================
  Files         125       90       -35     
  Lines       22964    20282     -2682     
===========================================
- Hits        16718     9272     -7446     
- Misses       6246    11010     +4764
Flag Coverage Δ
php-5.3-ubuntu-latest ?
php-5.4-ubuntu-latest ?
php-5.5-ubuntu-latest ?
php-5.6-ubuntu-latest ?
php-7.1-ubuntu-latest 45.71% <81.25%> (+0.02%) ⬆️
php-7.2-ubuntu-latest ?
php-7.3-ubuntu-latest ?
php-7.4-ubuntu-latest 45.65% <81.25%> (+0.01%) ⬆️
php-8.0-ubuntu-latest ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
include/barcodes/qrcode.php 63.88% <33.33%> (-0.34%) ⬇️
tcpdf.php 39.43% <85.18%> (-31.49%) ⬇️
include/barcodes/datamatrix.php 52.40% <100.00%> (+0.08%) ⬆️
include/tcpdf_static.php 35.28% <100.00%> (-18.26%) ⬇️
tcpdf_barcodes_1d.php 10.47% <0.00%> (-79.97%) ⬇️
include/tcpdf_colors.php 10.61% <0.00%> (-26.05%) ⬇️
tcpdf_barcodes_2d.php 65.95% <0.00%> (-10.02%) ⬇️
include/tcpdf_fonts.php 35.84% <0.00%> (-8.93%) ⬇️
... and 65 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c979d00...6b160ab. Read the comment docs.

@mvorisek
Copy link
Contributor Author

mvorisek commented Apr 3, 2021

@williamdes why PHP 5.4 and 5.5 tests are failing, can you fix that?

@williamdes
Copy link
Contributor

@williamdes why PHP 5.4 and 5.5 tests are failing, can you fix that?

I will try to have a look, as far as I can see the gd error that makes no sense

williamdes
williamdes reviewed Apr 3, 2021
View reviewed changes
phpstan.neon.dist Outdated Show resolved Hide resolved
@mvorisek
Copy link
Contributor Author

mvorisek commented Apr 7, 2021

@williamdes any feedback on fixing the CI?

@williamdes
Copy link
Contributor

@williamdes any feedback on fixing the CI?

Nope, but a recently merged PR also had the failure so I still have to search why it is failing, maybe a PHP extension setup issue :/

@williamdes
Copy link
Contributor

@williamdes any feedback on fixing the CI?

Nope, but a recently merged PR also had the failure so I still have to search why it is failing, maybe a PHP extension setup issue :/

Seems to make it: 930b1d0 (on #356)

@mvorisek mvorisek force-pushed the gh_add_phpstan branch 3 times, most recently from 45c4d4a to c30a757 Compare September 7, 2022 13:12
@mvorisek mvorisek force-pushed the gh_add_phpstan branch 2 times, most recently from e39701f to 8b1a58c Compare September 7, 2022 13:24
@mvorisek mvorisek requested review from williamdes and nicolaasuni and removed request for williamdes September 7, 2022 13:26
@mvorisek
Copy link
Contributor Author

mvorisek commented Sep 7, 2022

@williamdes please review

@nicolaasuni all feedback addressed & rebased on the latest main branch, please merge

@mvorisek mvorisek requested review from williamdes and removed request for nicolaasuni September 7, 2022 15:14
williamdes
williamdes previously approved these changes Sep 7, 2022
View reviewed changes
nicolaasuni
nicolaasuni previously approved these changes Dec 6, 2022
View reviewed changes
@nicolaasuni
Copy link
Member

Trying to merge this today but the static analysis test is failing:
https://github.com/tecnickcom/TCPDF/actions/runs/3627541867/jobs/6117531316

@mvorisek mvorisek dismissed stale reviews from nicolaasuni and williamdes via a458bc1 December 9, 2022 14:03
@mvorisek mvorisek requested review from williamdes and nicolaasuni and removed request for williamdes December 9, 2022 14:10
@mvorisek
Copy link
Contributor Author

mvorisek commented Dec 9, 2022

Trying to merge this today but the static analysis test is failing

@nicolaasuni fixed

@nicolaasuni nicolaasuni merged commit 37aa6ee into tecnickcom:main Dec 12, 2022
@mvorisek mvorisek deleted the gh_add_phpstan branch December 12, 2022 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@williamdes williamdes williamdes left review comments

@nicolaasuni nicolaasuni nicolaasuni approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@mvorisek @williamdes @robertotremonti @nicolaasuni @codecov-io