-
Notifications
You must be signed in to change notification settings - Fork 71
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
False positives on some bundle and classes #55
Comments
Another false positive not related to bundles: class: |
I updated the issue body. Except |
@maglnet do we have something to collect all defined symbols and the defining files? Would be interesting to have some debug output that dumps all files associated with the originating file. |
@soullivaneuh it seems that all the |
You are right, only some are reported.
It looks like not. But I found the I check that on some bundle of this list, and they all use this keyword. I think it's a clue. 👍 |
I don't think |
|
@Ocramius Well, is that really hard to manage Maybe this tool should also trow a warning about that? Going back to the issue. Some other bundle does not work simply because composer.json is not present. For example: https://github.com/dmaicher/doctrine-test-bundle/blob/ccdea2ce9fec5048385d1b9b5bc7c4c3f32ab48f/.gitattributes |
It's needed for some CI tools. See also: maglnet/ComposerRequireChecker#55
@soullivaneuh most of these issues would be fixed by switching to BetterReflection, I reckon. It's just not gonna be nice for performance, but it would be much, much more reliable :-) |
Sadly currently not, but this should be done soon as it would make debugging issues like this a lot easier and also gives a user hints where dependencies are found without the need to do a fulltext search within the code. |
It's needed for some CI tools. See also: maglnet/ComposerRequireChecker#55
It's a dev tool. IMHO, we can drop a bit of performance to make it more reliable. 😉 |
Another case: For this one, I don't know why composer-require-checker is yelling. The EDIT: Same thing for:
|
Concerning "autoload": {
"files": ["lib/swift_required.php"]
}, |
Concerning define('KERNEL_ROOT_DIR', __DIR__) And this php file is auto-loaded by composer, so it should not be an issue. |
Ok, I understand the problem with The second problem with the constant could be, that the autoloaded file requires another file and we currently do not follow |
Please see swiftmailer/swiftmailer#971 (comment).
The define function is on the "autoload": {
"psr-4": { "": "src/" },
"files": [
"app/env.php",
"app/AppKernel.php"
]
}, |
Another false positive since I split the Symfony fullstack requirement from my project:
But they are correctly required and installed under a dev env: "require-dev": {
"symfony/debug-bundle": "^4.0",
"symfony/var-dumper": "^4.0",
"symfony/web-profiler-bundle": "^4.0",
"symfony/web-server-bundle": "^4.0"
}, |
Are they used in |
@Ocramius It's quite more complicated, it's only bundle instantiation on |
Btw, yes, I fully understand where this comes from: I'm just saying that the exit code with this particular tool will always be |
I understand your point of view but I still think files like I won't require packages on production if I don't need them. Plus for that case, it will an issue for a lot of people following the Symfony standard. Adding a simple option listing the specific files where the tool should not care if they are require on dev env or not and let the user take the risk (or not) still worth it IMHO! :-) Shall I open a separate issue for that? |
Well, the |
I said it many, many, many many times: I don't care what
My simplistic suggestion (because every tool is opinionated, and these will always pop up a lot) is to suggest a feature in which we can map |
Yeah, your choice, I'm not trying to convince you. But yes, an option to permit both would solve everything! 👍 I'm on the issue. |
The problem regarding the constant from #55 (comment) should be fixed with 0.2.1 |
might be a workaround for some of maglnet#55's issues
might be a workaround for some of maglnet#55's issues
Hello, I had the same issue, with composer.json:
My command and his output:
My usage:
Best regards, |
Hi guys,
I think that a lot of these are false positives. For sure |
I don't see |
It's a dependency of |
@mmenozzi the idea of this tool ;) -> https://github.com/maglnet/ComposerRequireChecker#whats-it-about |
I swear that I've read it before submitting this comment. Now I just read it again twice and I understood. Sorry guys. |
Regarding |
|
Unfortunately, the way suggested by Symfony is one big array with all bundle classes as keys and sub-arrays for each one choosing the environment where it should be used. |
Yes, and you can split the development config into a separate file to be put somewhere outside your sources. |
i might have found a false-positive for |
One more false-positive: +-----------------+--------------------+
| unknown symbol | guessed dependency |
+-----------------+--------------------+
| IMAP\Connection | |
+-----------------+--------------------+
"require": {
"php": "^7.2 || ^8.0",
"ext-fileinfo": "*",
"ext-iconv": "*",
"ext-imap": "*",
"ext-mbstring": "*"
}, See https://github.com/barbushin/php-imap/blob/master/composer.json Would be also helpful, if I would just be able to "ignore" specific unknown symbols using the config file. |
Hi,
I'm not sure if it's a false positive.
If you're running the check with PHP versions < 8.1 the class is not available (seems like it was introduced with 8.1) and so it's a unknown symbol for the lower versions.
Ignoring symbols is already possible. Please have a look at the `symbol-whitelist` within the example configuration: https://github.com/maglnet/ComposerRequireChecker/blob/3.7.x/data/config.dist.json
|
Looks like the referenced project has never released a version that is compatible with PHP 8.1. I cannot identify what exactly is going on, but that link above currently reads
which is true for all versions 4.2.x, any earlier 4.x is allowing php ^7.2, and version 3.x states php >=5.5 or >=5.6. Looking at their bugtracker, apparently being compatible with PHP 8.1 is an open issue right now. |
This is still an issue.. This checker will only recognize it when spelled with a capital |
Sounds like a separate issue: perhaps send a patch with a test around case sensitivity? |
Command result (extract):
But the corresponding bundles are required on the root composer.json file:
The text was updated successfully, but these errors were encountered: