Generic/RequireStrictTypes: add XML documentation

[rodrigoprimo]

Description

This PR adds documentation for the Generic.PHP.RequireStrictTypes sniff.

Suggested changelog entry

Add XML documentation for the Generic.PHP.RequireStrictTypes sniff.

Related issues/external references

Part of #148

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]

Thanks @rodrigoprimo for this PR.

When looking at the actual sniff code and at the code samples, I'm wondering why you've elected to go for one standards block with one code comparison block ?

Some extra context about this sniff: #51 (recent change included in the 3.8.0 release)

[rodrigoprimo]

Thanks for checking this PR, @jrfnl!

When looking at the actual sniff code and at the code samples, I'm wondering why you've elected to go for one standards block with one code comparison block ?

Even though the sniff shows different errors/warnings for missing required strict_types and for strict_types found but disabled, I opted to use one <standard> and one <code_comparison> block as the valid examples would be essentially the same (similar to what we discussed in #177 and #159 (comment)).

Some extra context about this sniff: #51 (recent change included in the 3.8.0 release)

Upon seeing this PR, I realized that there are some valid cases that are not covered in the examples (case-insensitive execution directives, docblocks at the top of the file). Would you like me to add those?

[jrfnl]

Even though the sniff shows different errors/warnings for missing required strict_types and for strict_types found but disabled, I opted to use one <standard> and one <code_comparison> block as the valid examples would be essentially the same (similar to what we discussed in #177 and #159 (comment)).

The difference between those sniffs and this sniff is one of PHP functionality.

The other sniffs where checking for exactly one space in a certain place and whether there is too much space or too little space, the basic principle of the check is still the same.

That's not the case for this sniff. This sniff is checking for two closely related, but distinctly different things:

1. Is there a strict_types declaration ?
2. Is the strict_types declaration configured to enable strict_types ?

It could be perfectly valid for a project to require strict_types declarations, but not require them to be set to enabled (1). It might be rare and an edge-case, but it would still be perfectly valid.

In my mind, that means these two different rules need to be separated in the docs.

Some extra context about this sniff: #51 (recent change included in the 3.8.0 release)

Upon seeing this PR, I realized that there are some valid cases that are not covered in the examples (case-insensitive execution directives, docblocks at the top of the file). Would you like me to add those?

No, those kind of things are implementation details for the sniff to function correctly and do not belong in the docs.

[rodrigoprimo]

Thanks for the additional details, @jrfnl. After seeing your response, I agree that there is a significant difference between this sniff and the ones that I mentioned.

In my mind, that means these two different rules need to be separated in the docs.

There is one more thing that is making me unsure about separating these two rules in the docs that I want to check with you before proceeding with your suggestion. To facilitate the conversation, I pushed a temporary commit with a rough draft of how the XML documentation would look after the separation (3975274). Is this more or less what you have in mind?

It could be perfectly valid for a project to require strict_types declarations, but not require them to be set to enabled (1). It might be rare and an edge-case, but it would still be perfectly valid.

So far, all the documentation that I created for the sniffs describes only the default behavior of the sniff. I might be missing something, but I believe the behavior you described above is valid, but not possible without altering the default behavior and disabling Generic.PHP.RequiredStrictTypes.Disabled, no? My concern here is that this means that in the documentation, one of the valid examples for Generic.PHP.RequiredStrictTypes.MissingDeclaration (declare(<em>strict_types=0</em>);) is invalid when the sniff runs with the default configuration because of Generic.PHP.RequiredStrictTypes.Disabled.

[jrfnl]

@rodrigoprimo Sorry, I just realized I never got back to you after your comment. I'm going to leave some suggestions inline to see if we can move this PR forward.

[jrfnl]

Left some suggestions inline. Does that answer your questions sufficiently ?

[jrfnl]

Oh and I'm not sure the PHP open tags are needed in the code samples. The sniff (currently) just tries to find a declare() statement and doesn't actually check that it is the first statement in the file. (that might be something which could be added to the sniff, but is not currently checked)

[rodrigoprimo]

Thanks for the review, @jrfnl!

I pushed a commit implementing your suggestions and another one with two minor changes that occurred to me while checking the docs again. Similar to your suggestion here, I removed a redundant example and also changed the <em> tag position in one of the examples.

Oh and I'm not sure the PHP open tags are needed in the code samples. The sniff (currently) just tries to find a declare() statement and doesn't actually check that it is the first statement in the file. (that might be something which could be added to the sniff, but is not currently checked)

I believe I had added the PHP open tags wrongly assuming that the sniff checked the declare() statement is the first statement in the file. I have removed them for now and I can look into changing the sniff to check that in a separate PR.

Left some suggestions inline. Does that answer your questions sufficiently ?

I'm still unsure about adding an example that will throw an error when the sniff is run with the default configuration. But if that is not a problem, we can proceed.

[jrfnl]

I believe I had added the PHP open tags wrongly assuming that the sniff checked the declare() statement is the first statement in the file. I have removed them for now and I can look into changing the sniff to check that in a separate PR.

I probably should have opened an issue about that after merging PR #51.

Note: updating the sniff for this should not be underestimated. There are some notes under "Future scope" in PR #51 which detail some things to take into account/choices which would need to be made.

[jrfnl]

I pushed a commit implementing your suggestions and another one with two minor changes that occurred to me while checking the docs again. Similar to your suggestion here, I removed a redundant example and also changed the <em> tag position in one of the examples.

I'm still unsure about adding an example that will throw an error when the sniff is run with the default configuration. But if that is not a problem, we can proceed.

@rodrigoprimo Thanks for the update. LGTM! I think the examples as they are now are clear and instructive.

I'll rebase (without changes) and will merge this once the build has passed.