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

#354 Make sure language types are always in the symbol whitelist #355

Merged
merged 3 commits into from
Aug 30, 2022
Merged

#354 Make sure language types are always in the symbol whitelist #355

merged 3 commits into from
Aug 30, 2022

Conversation

willemstuursma
Copy link

@willemstuursma willemstuursma commented Aug 29, 2022

The language types should always be in the whitelist. Else, ComposerRequireChecker will give errors like:

The following 1 unknown symbols were found:
+----------------+--------------------+
| Unknown Symbol | Guessed Dependency |
+----------------+--------------------+
| mixed          |                    |
+----------------+--------------------+
Command exited with non-zero status 1

Copy link
Collaborator

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@Ocramius Ocramius added this to the 4.2.0 milestone Aug 29, 2022
@Ocramius
Copy link
Collaborator

Some CS changes needed

@willemstuursma
Copy link
Author

Some CS changes needed

Yes, I will take care of it.

@DanielBadura
Copy link
Contributor

Should we also remove the whitelist part on the default config json to prevent confusion on this topic?

@Ocramius
Copy link
Collaborator

Can certainly be done 👍

@willemstuursma
Copy link
Author

@DanielBadura @Ocramius Please let me know if this is satisfactory or feel free to merge.

@Ocramius Ocramius linked an issue Aug 30, 2022 that may be closed by this pull request
Copy link
Collaborator

@Ocramius Ocramius 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 one mutant to be fixed, indicating a missing test:

2) /home/runner/work/ComposerRequireChecker/ComposerRequireChecker/src/ComposerRequireChecker/Cli/Options.php:108    [M] UnwrapArrayUnique

--- Original
+++ New
@@ @@
          * Make sure the language types (e.g. 'true', 'false', 'object', 'mixed') are always included in the symbol
          * whitelist. If these are omitted the results can be pretty unexpected.
          */
-        $this->symbolWhitelist = array_unique(array_merge(self::PHP_LANGUAGE_TYPES, $symbolWhitelist));
+        $this->symbolWhitelist = array_merge(self::PHP_LANGUAGE_TYPES, $symbolWhitelist);
     }
     /**
      * @param array<string> $phpCoreExtensions

See https://github.com/maglnet/ComposerRequireChecker/runs/8087054988?check_suite_focus=true#step:9:74

@willemstuursma
Copy link
Author

Cheers, I added an assertion that should fix it

@Ocramius Ocramius self-assigned this Aug 30, 2022
@Ocramius Ocramius merged commit fb2a69a into maglnet:4.2.x Aug 30, 2022
@Ocramius
Copy link
Collaborator

Thanks @willemstuursma!

@willemstuursma willemstuursma deleted the 354-require-language-types branch August 30, 2022 12:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

mixed parameter type
3 participants