Verify the `installed_paths` after save

PHPCSStandards / composer-installer

Composer installer for PHP_CodeSniffer coding standards

https://packagist.org/packages/dealerdirect/phpcodesniffer-composer-installer

MIT License

559 stars 36 forks source link

Verify the `installed_paths` after save #97

Closed jrfnl closed 4 years ago

jrfnl commented 4 years ago

Proposed Changes

As most problems reported are to do with paths not being set correctly, let's verify success within the plugin, independently of PHPCS.

To that end, a new function verifySaveSuccess() has been added. This function takes the paths which were to be saved and compares them to the paths which are actually set in PHPCS after the save to determine whether or not the plugin was successful and changes the exit code if this is not the case.

Open questions:

Should additional debug output be generated by the verification function when verbose mode is turned on ? Something like Expected paths to be set to: %s, found: %s. This could also come in handy to verify that all paths which should be set were found by the plugin.

At this moment I don't have a test case for this new functionality, however, I imagine it will come in useful with future bug reports.

Potherca commented 4 years ago

Should additional debug output be generated by the verification function when verbose mode is turned on?

I know that goes against the principle of only reporting errors in output, but as this is in verbose mode, I would say Yes, definitely.

I imagine it will come in useful with future bug reports.

Fully agree. Should make life easier for all.

jrfnl commented 4 years ago

Oh dear ... while debugging why the build was failing, I've discovered a pretty interesting bug, which could well be the cause of some reported mystery issues. Win!

I'll work on a fix and send it in as a separate PR which will need to be merged before this PR as this PR will be dependent on it.

Potherca commented 4 years ago

PR #98 has been merged.

jrfnl commented 4 years ago

Thanks, I'll update this PR in a moment and will make it mergable if the build then passes.

jrfnl commented 4 years ago

Marking this ready for review. I've updated the PR for:

code layout
verbose output on failure

jrfnl commented 4 years ago

Just now thinking: this should probably also run cleanInstalledPaths() to safeguard against the comparison failing for Windows vs Linux slashes. I'll send in a new PR to add that.

jrfnl commented 4 years ago

Oh second thought: no, as $this->installedPaths is only updated when the path doesn't exist, cleanInstalledPaths() doesn't update for slash differences.

Hmm.. let's see how we fare for now. If needs be, this can be revisited later.