Create special case for T_BAD_CHARACTER constant

[fredden]

Fixes #1351

[jrfnl]

Hiya @fredden Thanks for looking into this!

I need to think this solution over a little as it is quite different from what I had in mind.

Would you mind explaining why you choose to create a separate array based sniff, instead of special casing the constant in the RemovedConstants and NewConstants sniffs ?

Do you expect this kind of thing to happen more regularly in PHP in the future ?

[fredden]

Would you mind explaining why you choose to create a separate array based sniff, instead of special casing the constant in the RemovedConstants and NewConstants sniffs ?

Initially I added more code to RemovedConstants to create a special case for this constant. When I came to do the same for NewConstants, I realised that I was writing a lot of (what looked like) duplicate code. Also, fitting these special cases into the existing unit tests seemed difficult.

If you'd prefer, I can look at re-doing this without adding a new sniff and instead only modifying RemovedConstants and NewConstants sniffs (maybe with a new trait to avoid duplication).

Do you expect this kind of thing to happen more regularly in PHP in the future ?

I have no idea. :) I suspect it won't re-occur, but I don't know.

[jrfnl]

Would you mind explaining why you choose to create a separate array based sniff, instead of special casing the constant in the RemovedConstants and NewConstants sniffs ?

Initially I added more code to RemovedConstants to create a special case for this constant. When I came to do the same for NewConstants, I realised that I was writing a lot of (what looked like) duplicate code. Also, fitting these special cases into the existing unit tests seemed difficult.

Just guessing - were you trying to make the special casing dynamic ? As in: allow for an array based list of special cases ? In that case, I can imagine there would be redundancy and code duplication, but as things are, we only need to account for one special case....

Do you expect this kind of thing to happen more regularly in PHP in the future ?

I have no idea. :) I suspect it won't re-occur, but I don't know.

Well, as neither of us can look into the future... what about writing the sniff based on the now ?

What I mean by that, is that the sniff as-is doesn't need to be array based.
It is only dealing with one specific constant and we know all we need to know about that constant, so unless (and until) a second constant would be removed and then re-added to PHP, there is no need for the sniff to have all the extra flexibility (and complexity) of being an array based sniff.

This would also allow for greatly simplifying the tests.

If you'd prefer, I can look at re-doing this without adding a new sniff and instead only modifying RemovedConstants and NewConstants sniffs (maybe with a new trait to avoid duplication).

Let me think it over a little more ;-)

In the mean time - would you like some general feedback on the code as-is ? (without an opinion on the direction of the PR yet)

[fredden]

were you trying to make the special casing dynamic ? As in: allow for an array based list of special cases ?

Yes, I had avoided hard-coding the T_BAD_CHARACTER name and listed this in a (array) class constant/property.

Well, as neither of us can look into the future... what about writing the sniff based on the now ?

What I mean by that, is that the sniff as-is doesn't need to be array based. It is only dealing with one specific constant and we know all we need to know about that constant, so unless (and until) a second constant would be removed and then re-added to PHP, there is no need for the sniff to have all the extra flexibility (and complexity) of being an array based sniff.

Looking at the code, there's not a whole lot in there that would be simpler if we hard-coded everything rather than looking up values from the array property. Perhaps I've misunderstood what you mean.

This would also allow for greatly simplifying the tests.

I can see that this would allow removing a few arguments in the data provider. I wanted to use yield in the data provider, which would have made it a lot cleaner, but the project still supports PHP 5.4 and yield was introduced in PHP 5.5.

It's possible to reduce the number of tests without impacting coverage - for example, we don't strictly need to test with all of 5.0 and 5.1 and 5.2 etc; picking only some versions would be enough to show the code works as expected.
Would you like me to reduce the tests?

In the mean time - would you like some general feedback on the code as-is ? (without an opinion on the direction of the PR yet)

Yes, that sounds useful for progressing this work.

[jrfnl]

As noted in my previous comment - these are mostly just general feedback/guidelines for how we work in this repo without an opinion on the direction this PR should take.

If we'd go with the new sniff, I would like to have a good think about the sniff name as well.
While it is a single-constant sniff it could just be named after the constant.
And maybe use Reintroduced instead of TemporarilyRemoved ?

[jrfnl]

@fredden and me discussed this off-GitHub and what with this being a breaking change as it currently is (new sniffname/error codes), while it could also be special cased in the NewConstants and RemovedConstants, I've stated a preference for doing the special casing instead and @fredden has indicated he's willing to work on a new iteration for this.

[jrfnl]

@fredden Thanks for updating the PR. Looks like this could still be simplified further.

Would you like me to push an update to the PR ?

[fredden]

@jrfnl thanks for the feedback. I've applied the suggested changes and confirmed that the test suite still runs without error. Would you like to squash-merge this, or shall I rewrite the history to have only one commit here (after you've reviewed again)?

[jrfnl]

@jrfnl thanks for the feedback. I've applied the suggested changes and confirmed that the test suite still runs without error. Would you like to squash-merge this, or shall I rewrite the history to have only one commit here (after you've reviewed again)?

@fredden Thanks for making that update and for your work on this PR! I'll squash-merge it once the build passes.
I did push a couple of tiny tweaks still to address earlier review remarks (#1586 (comment)), but nothing significant.

Feedback-wise, the only thing which I'd still say about this PR (but which is definitely not a blocker) is that splitting the tests and removing some redundancy would still be nice (see #1586 (comment) ), but I'm fine with leaving that.