Skip to content

Conversation

jrfnl
Copy link
Member

@jrfnl jrfnl commented Mar 12, 2025

Description

Includes minor bug fix to treat an empty value correctly.

Previously, passing --extensions= (without value) would result in the Config::$extensions property being set to:

public $extensions = [
    '' => 'PHP'
];

Now, an empty value will set the extensions array to an empty array.

public $extensions = [];

Note: in practice, the above change will not make a difference in how files are filtered/scanned.

👉 The change to the Config class will be most straight-forward to review while ignoring whitespace changes.

Suggested changelog entry

N/A

Includes minor bug fix to treat an empty value correctly.

Previously, passing `--extensions=` (without value) would result in the `Config::$extensions` property being set to:
```php
public $extensions = [
    '' => 'PHP'
];
```
Now, an empty value will set the extensions array to an empty array.
```php
public $extensions = [];
```

Note: in practice, the above change will not make a difference in how files are filtered/scanned.

:point_right: The change to the `Config` class will be most straight-forward to review while ignoring whitespace changes.
@jrfnl jrfnl force-pushed the feature/config-add-tests-for-extensions branch from d035cd0 to 25d0f75 Compare March 12, 2025 04:28
Copy link
Contributor

@rodrigoprimo rodrigoprimo left a comment

Choose a reason for hiding this comment

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

Looks good to me overall.

I just wonder if you considered adding a test for invalid extension values like --extensions=php/ and --extensions=/php? Those cases might require a fix similar to what was done here for --extensions=.

@jrfnl
Copy link
Member Author

jrfnl commented Mar 12, 2025

Looks good to me overall.

I just wonder if you considered adding a test for invalid extension values like --extensions=php/ and --extensions=/php? Those cases might require a fix similar to what was done here for --extensions=.

Slash handling is going away in 4.0 with the drop of the JS/CSS tokenizer, so as of 4.0, slashes in the values will throw an error (which is also why I added the tests as it will allow for having that change covered via the tests).

@rodrigoprimo rodrigoprimo self-requested a review March 13, 2025 13:38
Copy link
Contributor

@rodrigoprimo rodrigoprimo left a comment

Choose a reason for hiding this comment

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

That makes sense. Thanks for the additional details.

@jrfnl jrfnl added this to the 3.12.0 milestone Mar 13, 2025
@jrfnl jrfnl merged commit f91c044 into master Mar 13, 2025
72 checks passed
@jrfnl jrfnl deleted the feature/config-add-tests-for-extensions branch March 13, 2025 16:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants