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

Fix checkbashisms download ans SCA violations #6298

Merged
merged 1 commit into from Feb 18, 2022
Merged

Fix checkbashisms download ans SCA violations #6298

merged 1 commit into from Feb 18, 2022

Conversation

SpacePossum
Copy link
Contributor

No description provided.

@keradus
Copy link
Member

keradus commented Feb 18, 2022

sorry, Update devscript location vs content of PR is confusing. what location we gonna change + reasoning, can you share?

@coveralls
Copy link

coveralls commented Feb 18, 2022

Coverage Status

Coverage increased (+0.03%) to 93.211% when pulling abd054f on SpacePossum:master_fix_SCA into 51c8146 on FriendsOfPHP:master.

@jrmajor
Copy link
Contributor

jrmajor commented Feb 18, 2022

@keradus The changed variable VERSION_CB is used in wget -qO- "https://deb.debian.org/debian/pool/main/d/devscripts/devscripts_${VERSION_CB}.tar.xz". For the old value, it returns 404, so it looks like the location from which the script is downloaded will be changed.

@SpacePossum
Copy link
Contributor Author

all PR's are currently failing SCA set, for example:
https://github.com/FriendsOfPHP/PHP-CS-Fixer/runs/5232717511?check_suite_focus=true

this is because the location:

https://deb.debian.org/debian/pool/main/d/devscripts/devscripts_2.21.7.tar.xz

returns 404

the updated location

https://deb.debian.org/debian/pool/main/d/devscripts/devscripts_2.21.7~bpo11+1.tar.xz

works

However, since the install script of the SCA exits on the first error it hasn't run all the other tooling, like stan and psalm, either, which is what is the rest of the PR will be about.

@SpacePossum SpacePossum marked this pull request as draft February 18, 2022 12:06
@SpacePossum SpacePossum changed the title Update devscript location Fix checkbashisms download ans SCA violations Feb 18, 2022
public function offsetSet($index, $token): void
{
// @phpstan-ignore-next-line as we type checking here
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// @phpstan-ignore-next-line as we type checking here
/** @var mixed $token */

Also, there's treatPhpDocTypesAsCertain option in PHPStan configuration, which would prevent it from reporting runtime type checks for variables that are typed in phpdocs, although given that the entire project is statically analyzed, I think turning it off would do more harm than good.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Switching it off on raises a 3 items on this PR, maybe it is worth doing that? Not sure....

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think so. In most methods we trust PHPDoc types. If there are only a few places where we want to validate them at runtime, it's safer to add @var mixed, as that is how we treat this variable in this case. Switching treatPhpDocTypesAsCertain off would prevent PHPStan from reporting truly unnecessary checks that are there by mistake.

@SpacePossum SpacePossum marked this pull request as ready for review February 18, 2022 13:06
@SpacePossum SpacePossum merged commit 1157777 into PHP-CS-Fixer:master Feb 18, 2022
@SpacePossum SpacePossum deleted the master_fix_SCA branch February 18, 2022 14:17
@SpacePossum
Copy link
Contributor Author

lets get the PR's green first with this one, than we can refine in others if really needed

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.

None yet

4 participants