Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add rules for debug statements in app/test/blade #23

Merged
merged 17 commits into from
Jun 18, 2024

Conversation

Treggats
Copy link
Contributor

@Treggats Treggats commented Jun 11, 2024

Sometimes you forget to remove debug statements. This PR adds rules to prevent or spot those occurences.

@Treggats Treggats self-assigned this Jun 11, 2024
@Treggats Treggats force-pushed the feature/add-debug-statements-rules branch 3 times, most recently from 4ae502f to cf65a3c Compare June 11, 2024 17:56
private static function hasDisallowedStatements(string $text): bool
{
return match (true) {
str_contains($text, '@dump') => true,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$text is the template, so using str_contains(..) to find currences. Can't, as far as I know, use an AST for this

$this->analyse([__DIR__ . '/stubs/debug-in-view.blade.php'], [
[
'No debug statements should be present in blade files.',
1,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will always be on the first line, because of the str_contains(..)

@Treggats Treggats marked this pull request as ready for review June 11, 2024 18:02
@Treggats Treggats force-pushed the feature/add-debug-statements-rules branch from 1bf1ef5 to 9894194 Compare June 13, 2024 14:54
@Treggats Treggats force-pushed the feature/add-debug-statements-rules branch 3 times, most recently from db2c72f to 6b64e42 Compare June 13, 2024 16:02
count: 1
path: tests/Rules/stubs/UseSpatieInvadeInsteadOfLivewire.php
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stubs are no longer checked, so not needed anymore

path: src/Rules/Debug/ChainedNoDebugInNamespaceRule.php

-
message: "#^Access to an undefined property PhpParser\\\\Node\\:\\:\\$name\\.$#"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PHPStan could not understand that the name property existed and I didn't how else to tell him that

* @template T of Node
* @implements \PHPStan\Rules\Rule<T>
*/
abstract class BaseNoDebugRule implements Rule
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

base class with common methods over several classes

*/
abstract class BaseNoDebugRule implements Rule
{
protected string $message;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

holds the message (format) that PHPStan will report when errors are encountered

{
protected string $message;

protected array $haystack = [
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

default debug statements


protected function hasDisallowedStatements(string|array $statement): bool
{
if (is_array($statement)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

caters to both Blade and Namespace rules

return null;
}

return match (strtolower($namespace)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

allows re-use with different namespaces

{
$this->haystack = [
...$this->haystack,
'ddd',
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

expand on the default debug statements

@Treggats Treggats dismissed SanderMuller’s stale review June 13, 2024 16:06

implemented requested change

@Treggats Treggats force-pushed the feature/add-debug-statements-rules branch from 6b64e42 to 91ef3d1 Compare June 13, 2024 16:06
return StaticCall::class;
}

public function processNode(Node $node, Scope $scope): array

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method is identical to the one in ChainedNoDebugInNamespaceRule. Maybe it's something that can be extracted to a parent class.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have thought of introducing a base class, as the lines 29-43 also are pretty much the same.

I chose not to for the sake of being explicit in what the rule does. I also like that I can see upon opening of this file what the checks are and what node is being processed

@Treggats Treggats merged commit 9c9b3ee into main Jun 18, 2024
10 checks passed
@Treggats Treggats deleted the feature/add-debug-statements-rules branch June 18, 2024 07:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants