researchgate / phpnsc

Small tool to check for missing use statements when using PHP namespaces
MIT License
43 stars 4 forks source link

Integration with PHPMD #9

Open josephdpurcell opened 8 years ago

josephdpurcell commented 8 years ago

Have you considered integration with PHPMD?

I am interested in helping make this compatible with PHPMD.

bashofmann commented 8 years ago

We are not using phpmd actively, so that was never considered. Feel free to work on it and send a pull request, though it should still work kind of stand alone as well. Also keep in mind that because of https://github.com/researchgate/phpnsc/issues/7 there are plans to switch the whole parsing logic completely to https://github.com/nikic/PHP-Parser, so maybe we could try to combine all this work.

josephdpurcell commented 8 years ago

Currently the PHP world has no ability to analyze for missing use statements, you have to do it through human observation or execution of the code. This is a little absurd considering how obscure the problem is (i.e. its hard to identify and debug) and how critical it is (i.e. execution fails).

Integrating this into PHPMD or PHPCS would open the door for editors like Vim and Sublime or CI tools like CodeClimate or Travis would be able to use it without adding a dependency or writing an additional plugin (i.e. there already exists integrations with PHPMD and PHPCS for the editors and tools listed above). As such, I believe this work would get greater exposer and provide greater value by integrating with PHPMD or PHPCS.

Observations

Commentary

First, I want to make a side note. I respect the desire to allow this to continue to be a standalone utility and I don't want to sway that. That said, I do want to note that it may be less effort overall to integrate with an existing static analysis tool (PHPCS or PHPMD) while providing more value (editors and tools already integrate with PHPCS and PHPMD).

Second, I want to outline what I see enabling the ability to integrate with PHPCS or PHPMD:

  1. I believe the only change to code that needs to happen is the analyzeFile method (and class it exists on) be rewritten to not be dependent upon any parser, i.e. have an efferent coupling of 0. I imagine this would look something like passing the contents of the file and available fully qualified class names (FQCN) as method parameters. Then, let resultant errors be a either a return value or use an observer pattern.
  2. Add a PHPCS or PHPMD config file to the repository that could then be referenced by someone wanting to use this repo as a sniff or mess detection rule, see Drupal coder as a PHPCS example and Silverstripe as a PHPMD example. These could be in a directory as a sibling to src to keep the code segregated if desired, or if namespaced properly could also live inside of src.

Next Steps

  1. Identify if there is interest in doing the refactorings mentioned in points 1 and 2 in the Commentary section.
  2. Identify if PHPCS or PHPMD is a better integration.

What are your thoughts?

bashofmann commented 8 years ago

Since the tool works for us as it is, there is not a huge priority for us to work on this. But feel free to fork the tool and integrate it with PHPCS or PHPMD. But also keep in mind that some functionality, e.g. knowing if a class exists or not and therefor if the use statement is correct or not will probably not work with PHPCS since as far as I know it just works on a file by file base. I don't know enough about the internals of PHPMD to judge if it would be possible.

ravage84 commented 5 years ago

Hi @josephdpurcell & @bashofmann

I just wanted to let you know, that since the end of July, with PHPMD 2.7.0, it gained the capability to find missing class imports.

https://github.com/phpmd/phpmd/releases/tag/2.7.0 https://phpmd.org/rules/cleancode.html#missingimport

josephdpurcell commented 5 years ago

@ravage84 Thank you for sharing this! What a great addition to phpmd!

I think a good next step would be to compare the behavior of PHPMD 2.7.0 missingimport check and this tool. Based on what @bashofmann said, it sounds like PHPMD has a severe limitation in that it checks only file-by-file. I know the limitations of popular PHP parsing tools like PHPMD are significant pain points for various projects, and are the cause of new parsers and analysis tools.

As a next step it would be good to see a comparison between the two tools.

ravage84 commented 5 years ago

Based on what @bashofmann said, it sounds like PHPMD has a severe limitation in that it checks only file-by-file.

That is correct. And for PHPMD it is very unlikely, that this can be changed.

As a next step it would be good to see a comparison between the two tools.

Feel free to do so. The mentioned rule is just one among many for us. We have no specific focus on that functionality.

josephdpurcell commented 5 years ago

@ravage84 I created https://github.com/josephdpurcell/phpmd-phpnsc-comparison for some quick testing.

I am getting less errors than I would have expected. I've created https://github.com/josephdpurcell/phpmd-phpnsc-comparison/issues/1 to track that.

I would like to help do a comparison of the tools by having a fabricated codebase that phpmd and phpnsc can be run on. Do you have suggestions on why it's not showing more errors? (see the TODO statements throughout the src dir)

ravage84 commented 5 years ago

I would like to help do a comparison of the tools by having a fabricated codebase that phpmd and phpnsc can be run on.

For such a fabricated codebase I would recommend you to think about al the ways a class can be used in a file and create some fake code to trigger those erros.

Do you have suggestions on why it's not showing more errors? (see the TODO statements throughout the src dir)

If you talka bout phpnsc, no. I never used this tool. As for PHPMD, the rule is new and likely doesn't cover everything, yet.

josephdpurcell commented 5 years ago

Ok, thanks @ravage84 .

@bashofmann Perhaps you can help with the comparison between PHPCS and PHPNSC? I have commented on https://github.com/josephdpurcell/phpmd-phpnsc-comparison/issues/1#issuecomment-519607192 with a list of errors I would expect to see. I'm wondering if my configuration is wrong? https://github.com/josephdpurcell/phpmd-phpnsc-comparison/blob/master/phpnsc_config.json

Looking at vendor/rg/phpnsc/test/rg/tools/phpnsc/NamespaceDependencyCheckerTest.php I see that there are checks for some of these things, but they aren't being triggered.