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

Config: add tests for setting --extensions #872

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

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).

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