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

feat: Introduce variable_case fixer #5097

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

jmkoni
Copy link

@jmkoni jmkoni commented Aug 7, 2020

The project that I am working in has a rule that variables should all be in camelCase. However, we don't have any way to currently lint or automatically fix. Since we are using PHPCSFixer, I wanted to add a fixer that would automatically make that change.

@GrahamCampbell
Copy link
Contributor

This fixer should be marked as risky. It will break compact/require/include.

@GrahamCampbell
Copy link
Contributor

Also, we are missing test cases with string variable interpolation?

@jmkoni
Copy link
Author

jmkoni commented Aug 7, 2020

@GrahamCampbell do you mean a test like this:

[
     '<?php $testVariable = 2; echo "hi $testVariable!"; ',
     '<?php $test_variable = 2; echo "hi $test_variable!"',
]

It will break compact/require/include
Is there something I can do to make sure it is not risky? New-ish to PHP.

@GrahamCampbell
Copy link
Contributor

Yes, and "${testVariable}" and "{$testVariable}". Both cases tokenize differently, and need handling.

@julienfalque
Copy link
Member

Is there something I can do to make sure it is not risky? New-ish to PHP.

Sadly no. PHP CS Fixer works on one file at a time and isn't able to detect that a change in that file will have any impact on other files. Users have to check for breaking changes after running risky rules.

You may even have cases where a break is introduced inside the same file:

$some_variable = 'foo';
$name = 'some_variable';
echo $$name; // prints "foo"

would become:

$someVariable = 'foo';
$name = 'some_variable';
echo $$name; // Undefined variable $some_variable

PHP CS Fixer does not really have advanced analysis capabilities and detecting this would be too much complicated to implement here IMO. Maybe tools like PHPStan or Psalm can detect it anyway.

@GrahamCampbell
Copy link
Contributor

Even then, this kind of fixer is inherently risky, like I said earlier. It cannot fix the following correctly:

function f($bar_baz, $file)
{
    require $file;
}

@jmkoni
Copy link
Author

jmkoni commented Aug 7, 2020

I updated this, marked it as risky, added some more tests.

@keradus keradus changed the base branch from 2.16 to 2.17 December 17, 2020 13:44
@keradus keradus changed the base branch from 2.17 to 2.18 January 24, 2021 21:34
@spawnia
Copy link

spawnia commented Apr 6, 2021

Thanks @jmkoni for your work on this PR. We are trying to add this as a custom rule via mll-lab/php-cs-fixer-config#3 and stumbled upon some edge cases:

$_ = 2;
var_dump($_ENV);

Thus, i modified the camel case fixer to preserve leading underscores and exclude globals.

    private function camelCase(string $string): string
    {
        $leadingDollar = '';
        if ('$' === $string[0]) {
            $string = substr($string, 1);
            $leadingDollar = '$';
        }

        // Preserve leading underscores
        $leadingUnderscoreMatches = [];
        Preg::match('/^_*/', $string, $leadingUnderscoreMatches);
        $leadingUnderscores = $leadingUnderscoreMatches[0] ?? '';

        if (array_key_exists($string, $GLOBALS)) {
            return $leadingDollar . $string;
        }

        $string = Preg::replace('/[$_]/i', ' ', $string);
        $string = trim($string);
        // uppercase the first character of each word
        $string = ucwords($string);
        $string = str_replace(' ', '', $string);

        return $leadingDollar . $leadingUnderscores . lcfirst($string);
    }

@keradus keradus changed the base branch from 2.18 to 2.19 May 3, 2021 22:11
@muglug
Copy link

muglug commented Jun 4, 2021

This also converts property names, which may not be desired. To avoid that I changed the applyFix method locally:

    /**
     * {@inheritdoc}
     */
    protected function applyFix(\SplFileInfo $file, Tokens $tokens)
    {
        foreach ($tokens as $index => $token) {
            if ((T_VARIABLE === $token->getId()) || (T_STRING_VARNAME === $token->getId())) {
                $prev_index = $tokens->getPrevMeaningfulToken($index);
                $prev_token = $tokens[$prev_index];

                if ($prev_token->getId() === T_STATIC) {
                    $prev_index = $tokens->getPrevMeaningfulToken($prev_index);
                    $prev_token = $tokens[$prev_index];
                }

                if ($prev_token->getId() === T_PROTECTED
                    || $prev_token->getId() === T_PUBLIC
                    || $prev_token->getId() === T_PRIVATE
                    || $prev_token->getId() === T_VAR
                    || $prev_token->getId() === T_DOUBLE_COLON
                ) {
                    continue;
                }

                $tokens[$index] = new Token([$token->getId(), $this->updateVariableCasing($token->getContent())]);
            }
        }
    }

@keradus keradus changed the base branch from 2.19 to master August 2, 2021 18:10
@shakaran
Copy link
Contributor

shakaran commented Sep 4, 2021

@jmkoni could you rerun

php dev-tools/doc.php

For update README.rst and resolve the conflict, so it would look RTM for maintainers? Thanks

@Wirone Wirone added the status/to verify issue needs to be confirmed or analysed to continue label Jun 6, 2023
@Wirone Wirone added the status/rebase required PR is outdated and required synchronisation with main branch label Jul 2, 2023
@Wirone Wirone removed status/to verify issue needs to be confirmed or analysed to continue status/rebase required PR is outdated and required synchronisation with main branch labels Jul 11, 2023
@Wirone Wirone changed the title Add variable name rule feature: Add variable name rule Jul 11, 2023
@Wirone Wirone changed the title feature: Add variable name rule feature: Introduce variable_case fixer Jul 11, 2023
@Wirone
Copy link
Member

Wirone commented Jul 11, 2023

I've added failing test cases for class' properties to address @muglug's comment - will fix it after initial agreement that we want this fixer.

@Wirone Wirone changed the title feature: Introduce variable_case fixer feat: Introduce variable_case fixer Dec 11, 2023
@Wirone Wirone requested a review from keradus December 12, 2023 01:46
@Wirone
Copy link
Member

Wirone commented Dec 12, 2023

@keradus we need decision whether we want to continue to work on this (see this comment).

This comment was marked as outdated.

This comment was marked as outdated.

@github-actions github-actions bot added the status/to recover PRs that were closed without being merged, but can be refreshed and continued label Mar 28, 2024
@github-actions github-actions bot closed this Mar 28, 2024
@Wirone Wirone reopened this Mar 28, 2024
@Wirone Wirone removed status/stale status/to recover PRs that were closed without being merged, but can be refreshed and continued labels Mar 28, 2024
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.

None yet

7 participants