Ensure that Fixer::endChangeset() returns the real outcome

[stronk7]

Description

I was debugging a complex sniff that implies multiple potential modifications to the same token (that is, right now, not possible - we could discuss about that another day), when have detected that the Fixer::endChangeset() method is returning always true, no matter the changes have been not accepted.

So this just ensures that the method returns the real outcome.

I've been looking to current tests to try to add something to have it covered, but it seems that the Fixer is one of those areas needing some coverage, haven't found any test explicitly covering it.

Also, I've searched at github, to see if anybody may be using expressions like:

$outcome = $phpcsFile->fixer->endChangeset()...
if ($phpcsFile->fixer->endChangeset()...

And have found zero lines of code using the return value of the method. Hence, I think it's a safe change to apply.

Suggested changelog entry

Ensure that Fixer::endChangeset() returns the real outcome

Related issues/external references

Fixes N/A

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
  • This change is only breaking for integrators, not for external standards or end-users.
• Documentation improvement

PR checklist

• I have checked there is no other PR open for the same change.
• I have read the Contribution Guidelines.
• I grant the project the right to include and distribute the code under the BSD-3-Clause license (and I have the right to grant these rights).
• I have added tests to cover my changes.
• I have verified that the code complies with the projects coding standards.
• [Required for new sniffs] I have added XML documentation for the sniff.

[jrfnl]

@stronk7 Thanks for this PR and the due diligence of the codesearch.

I'm a bit hesitant to accept this change, largely for the reasons you also mention above (no tests, potential for breaking code in external standards), but also because there actually is a code-path which returns false - if ($this->inConflict === true) - and doesn't clear the current changeset, while now there would be a codepath which returns false and would clear the current changeset (but also changes inChangeset to false).

For the record, I've also done a codesearch via Sourcegraph and can confirm that, in practice, this change is very unlikely to break anything.

Digging into the code history, it looks like the return true was only added relatively recently and was added for consistency reasons more than anything else (it used to be false|void) and I can't find a mention of the change in the changelog either.

All in all, I'm inclined to accept the change, but for the 4.0 branch with a "breaking change" label and preferably after tests have been added.

I made a start adding tests for this class recently, when I wanted to make a change to one of the methods in this class myself and I'd encourage you to add tests covering the code being changed as well.

[fredden]

I don't understand the value of returning any value from this method. I can understand that the return value could be used to indicate if a particular set of changes were applied or not, but I don't see what a sniff could usefully do with that information.

If a sniff is trying to detect conflicts, then by the time endChangeset() is called, it's too late to go back and decide to not call addFixableWarning/addFixableError.

If a sniff is modifying the same content / set of tokens multiple times within itself, then that's a design flaw which can be fixed within the sniff. (Set the correct set of tokens / content just once.) The fact that a particular change-set was rejected isn't something that the sniff can usefully do anything about.

I advocate for making this method return void instead of bool.

[stronk7]

Ok,

I just proposed this because I faced the problem when debugging a complex "recursive" Sniff (in charge of sorting some lines alphabetically). And it drove me crazy not understanding why the fixer was not fixing anything and, still, returning true.

Finally, I workaround it by "accumulating" all the recursive changes in a structure and then, applying them all-together, escaping from the "conflict-and-rollback" behaviour of the fixer (and the 50 iterations limit).

Yes, I saw that the return true; was added apparently unrelated to anything meaningful, heh.

In any case, I'm happy closing this as won't fix or, if you think it's worth it, look for tests in 4.0 and try to complete the PR.

100% you preference. Ciao :-)

Edited: Sorry @fredden, did not see your comment! About your proposal of changing it to return void and forget... yeah, it's an alternative. Although: 1) I still find some value into knowing if a changeset worked or no (it was useful for me when debugging/logging information in my recursive madness) and 2) the change also will require the same verifications and tests to fix the new behaviour - surely simpler, heh, but still.

[jrfnl]

@stronk7 Thanks for your response and @fredden for your input.

I'd suggest we mark this PR as "blocked" until a more complete set of tests for the Fixer class have been added. That okay with you ?

[stronk7]

That okay with you ?

Sure-it-is!