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

Fixed typo when checking if the field diffFile is empty. #99

Merged
merged 1 commit into from
May 23, 2024
Merged

Fixed typo when checking if the field diffFile is empty. #99

merged 1 commit into from
May 23, 2024

Conversation

dmitr1y
Copy link
Contributor

@dmitr1y dmitr1y commented May 23, 2024

After moving the options to CliOptions in commit d7eb116, the field diff was renamed to diffFile, which caused a bug in the handler for the --diff argument.

After moving the option to `CliOptions` in commit d7eb116, the field `diff` was renamed to `diffFile`, which caused a bug in the handler for the `--diff` argument.
@sirbrillig
Copy link
Owner

Wow, nice find. That must have totally broken manual mode. Interesting that static analysis missed that; possibly because empty() works on undefined values? I'll check. There's some static analysis errors but I think they might just be because Psalm has become more strict. I'll make sure that's the case and if so I can fix them in a different PR.

@sirbrillig sirbrillig merged commit 3811a96 into sirbrillig:trunk May 23, 2024
12 of 13 checks passed
@sirbrillig
Copy link
Owner

sirbrillig commented May 23, 2024

Interesting that static analysis missed that; possibly because empty() works on undefined values? I'll check.

Yup. That's the case. If I remove the empty(), then $this->diff shows an error. I may remove all those empty() checks.

ERROR: UndefinedThisPropertyFetch - PhpcsChanged/CliOptions.php:369:8 - Instance property PhpcsChanged\CliOptions::$diff is not defined (see https://psalm.dev/041)
                        if (!$this->diff || empty($this->phpcsUnmodified) || empty($this->phpcsModified)) {

sirbrillig added a commit that referenced this pull request May 23, 2024
Since `empty()` works on null undefined without a warning, it also hides
bugs from static analysis (causing the bug in
#99). This change should
have the same effect while not ignoring warnings.
sirbrillig added a commit that referenced this pull request May 23, 2024
* Replace empty() calls in CliOptions

Since `empty()` works on null undefined without a warning, it also hides
bugs from static analysis (causing the bug in
#99). This change should
have the same effect while not ignoring warnings.

* Replace `empty()` in other files with more explicit checks

Again, this is because `empty()` allows undefined values and hides
potential bugs from static analysis.

* Replace isset with array_key_exists in Cli

* Replace isset in addCacheEntry

* Replace isset with array_key_exists in CliOptions->fromArray

* Replace isset in CliOptions->toArray

* Replace isset with array_key_exists in FullReporter

* Replace isset in getPhpcsStandardOption

* Use boolval to make CliOptions->toArray more explicit

* Make json_encode guard more explicit in JsonReporter

* Make fileName guard in LintMessages more explicit

* Use more explicit guards in some UnixShell getters

* Make getExecutablePath guards more explicit

* Make getExecutablePath even more explicit

* Add line breaks for preg_match to debug which condition is a problem

psalm reports `RiskyTruthyFalsyComparison` for this line but I can't
figure out why.

* Replace `empty()` in UnixShell->getPhpcsVersion
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