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

Normalize leading backslashes in banned function names #30

Merged
merged 6 commits into from
Oct 11, 2023
Merged

Normalize leading backslashes in banned function names #30

merged 6 commits into from
Oct 11, 2023

Conversation

spawnia
Copy link
Contributor

@spawnia spawnia commented Feb 18, 2020

I stumbled upon this when trying to ban the use of sprintf in our project. Because we are using the amazing https://github.com/thecodingmachine/safe, we also have to ban \Safe\printf.

The configuration looks like this:

  banned_code:
    nodes:
    - type: Expr_FuncCall
      functions:
      - sprintf
      - \Safe\sprintf

What happened is that \Safe\sprintf was not found. Upon further investigation, i found that php-parser strips leading backslashes from function calls, even when they occur in the source code.

To avoid this hard to find error, i applied the same normalization to the banned functions definitions from the configuration file.

@spawnia
Copy link
Contributor Author

spawnia commented Apr 28, 2020

@mremi do you mind taking a look at this PR?

@spawnia
Copy link
Contributor Author

spawnia commented Oct 11, 2023

@mremi I just resolved the merge conflicts. After 3 years, can I get some feedback?

@szepeviktor
Copy link

@celcius112 Do you have someone at Ekino to look at this PR?

@mremi
Copy link
Member

mremi commented Oct 11, 2023

Hi @spawnia sorry for the delay, thank you for this contribution 👍 , could you please update CHANGELOG.md and squash your commits?

@spawnia
Copy link
Contributor Author

spawnia commented Oct 11, 2023

Added a changelog entry. Can you just use GitHub's Squash & Merge? I am having a hard time getting git to do what I need it to.

@mremi mremi merged commit 65a5752 into ekino:master Oct 11, 2023
3 checks passed
@mremi
Copy link
Member

mremi commented Oct 11, 2023

Done, thanks again

@spawnia spawnia deleted the normalize-function-names branch October 11, 2023 08:49
* @param array<string, mixed> $bannedNodes
* @return array<string>
*/
protected function normalizeFunctionNames(array $bannedNodes): array

Choose a reason for hiding this comment

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

From the method name to see if it should not have a return value and there is no need to pass parameters.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mutating parameters is neither common nor good practice in PHP and requires passing by reference for array. Seems bad.

Copy link

@kayw-geek kayw-geek Oct 19, 2023

Choose a reason for hiding this comment

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

Wouldn't the following be better practice?

    public function __construct(array $bannedNodes)
    {
        $this->bannedNodes     = array_column($bannedNodes, null, 'type');
        $this->bannedFunctions = self::normalizeFunctionNames();
    }
  protected function normalizeFunctionNames(): void
    {
        $this->bannedFunctions =  array_map(
            static function (string $function): string {
                return ltrim($function, '\\');
            },
            $this->bannedNodes['Expr_FuncCall']['functions'] ?? []
        );
    }

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 think it would be much worse, the signature (): void expresses no intent.

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