ETA v3?

[Levivb]

Hey,

Do you have an ETA for when v3 can have a stable tag?
arrow functions are not possible if I recall correctly due to constrains in this package

Regards,
Levi

[sirbrillig]

We're just waiting on PHPCSUtils to get a stable tag, but as that is unlikely to happen in the near future, I'm working on backporting the PHP 7.4 support to v2.x here: #213

I'm going to try to get to that this weekend.

[sirbrillig]

@Levivb if you are able, testing https://github.com/sirbrillig/phpcs-variable-analysis/releases/tag/v2.10.0-beta.1 would be very helpful!

[Levivb]

Sure thing

I ran the v2.10.0-beta.1 tag in a 690 files repository and got the following additional reports which first weren't reported:

163 | WARNING | Unused variable $value.

        array_walk($userNameParts, static function (string &$value): void {
            if (strlen($value) <= 3) {
                return;
            }

            $value = ucfirst($value);
        });

This is a false positive since the $value variable is passed by reference. So it actually is changed in the $userNameParts variable

In another file the same:
80 | WARNING | Unused variable $value.

        array_walk_recursive($input, function (&$value): void {
            foreach ($this->settings['manipulators'] as $manipulator) {
                if ($value === null) {
                    continue;
                }
                $value = $this->$manipulator($value);
            }
        });

Also a false positive due to the pass by reference of $value

And the third, the same:
175 | WARNING | Unused variable $value.

        array_walk_recursive($array, static function (&$value): void {
            if (!is_object($value)) {
                return;
            }

            $value = clone $value;
        });

So that's actually just one bug which has three occurrences in our code

in another project a bit more complicated example, but the same thing:
111 | WARNING | Unused variable $item.

        $nestedItemsToArray = static function ($array, $maxDepth = 1, $level = 1) use (&$nestedItemsToArray): array {
            array_walk($array, static function (&$item) use (&$nestedItemsToArray, $maxDepth, $level): void {
                $item = (array)$item;

                if ($maxDepth - 1 <= 0) {
                    return;
                }

                $item = $nestedItemsToArray($item, $maxDepth - 1, $level + 1);
            });

            return $array;
        };

In a third project, no false positives.

So as far as I can see in three sizeable projects there's just this one bug.

Of course I can't see any false negatives

Not sure, maybe this should also be reported since the first assignment is immediately overwritten?

                $attributes = 4;
                $attributes = (array)$item;

[Levivb]

ow, btw to be complete.

This is the configuration of VariableAnalysis.CodeAnalysis.VariableAnalysis:

<rule ref="VariableAnalysis.CodeAnalysis.VariableAnalysis">
        <properties>
            <property name="allowUnusedFunctionParameters" value="true"/>
            <property name="allowUnusedCaughtExceptions" value="true"/>
            <property name="allowUnusedParametersBeforeUsed" value="true"/>
            <property name="allowUndefinedVariablesInFileScope" value="true"/>
            <property name="validUndefinedVariableNames" value="router factory"/>
            <property name="allowUnusedForeachVariables" value="false"/>
            <property name="allowWordPressPassByRefFunctions" value="false"/>
        </properties>
    </rule>

[sirbrillig]

Thanks for that excellent testing! I'll take care of those regressions as soon as I'm able.

[sirbrillig]

I made a separate issue for this bug here: #215 since I believe it also affects 3.0.

[sirbrillig]

@Levivb Could you try https://github.com/sirbrillig/phpcs-variable-analysis/releases/tag/v2.10.0-beta.3 to see if there's any other issues you can see?

[Levivb]

Ow, sorry for not getting back to you. This notification kinda drowned a bit in my mail.
I'll check it out today :)

Do note that my implementation does not test all features of this sniff due to the configuration (as noted in #214 (comment))

Especially:

            <property name="allowUnusedFunctionParameters" value="true"/>
            <property name="allowUnusedCaughtExceptions" value="true"/>
            <property name="allowUnusedParametersBeforeUsed" value="true"/>
            <property name="allowUndefinedVariablesInFileScope" value="true"/>

[sirbrillig]

No worries! My own testing hasn't shown anything problematic. I mostly want to be sure that the false positives you saw are gone. I'm sure there's more bugs (there always are) but they can be addressed in patches.

[Levivb]

Bit late to the party, since 2.10 is already out. But no false positives anymore. So that's good 👍

I did notice phpcs runs about 10% longer overall and on one specific file of 1200 lines the time phpcs takes to check it even increases to 90 seconds (normally it's one second). I've yet to pinpoint if there's a specific piece of code that triggers that delay. So I'll get back to you on that.

[sirbrillig]

Thanks! That's really helpful to know. I previously had a major performance issue which I was able to track down and fix thanks to a copy of a file that triggered it (a 7487 line file that took 2 minutes and now takes around 2 seconds); if your 1200 line file happens to be something you can safely share with me, that would be very helpful! In the meantime I'll see if just running performance checks on the files I have can find anything.

[Levivb]

Sure thing

I trimmed the file down to it's relevant part.

It seems Rule::in(isset($var) ? $var->call1()->call2() : []), takes about 7 seconds each.

File collapsed for clarity in this thread

<php

declare(strict_types=1);

namespace App;

use Illuminate\Http\Request;
use Illuminate\Validation\Rule;

final class Apply
{
    /** @var array */
    private $data = [];

    /** @var Request|null */
    private $request;

    /**
     * @return array
     */
    protected function validationRules(): array
    {
        $civilServantId = $this->data['civilServantOptions']->where('target', true)->first()->id;
        $fileUploadValidationRule = 'dummy';

        $result = [
            'application' => [
                // Default
                'screen' => 'bail|string|required|in:mobile,tablet,desktop,unknown',
                'ga_client_id' => 'bail|string|max:150',
                'initials' => 'bail|string|required|alpha_extended|max:50',
                'first_name' => 'bail|string|required|alpha_extended|max:255',
                'last_name_prefix' => 'bail|string|alpha_extended|max:255',
                'last_name' => 'bail|string|required|alpha_extended|max:255',
                'gender' => [
                    'bail',
                    'string',
                    'required',
                    Rule::in(isset($this->data['genders']) ? $this->data['genders']->pluck('id')->all() : []),
                ],
                'birthdate' => [
                    'bail',
                    'string',
                    'date_format:d-m-Y',
                    'before:' . date('d-m-Y', strtotime('-15 years')),
                    'after:' . date('d-m-Y', strtotime('-99 years')),
                ],
                'country' => [
                    'bail',
                    'string',
                    'required',
                    Rule::in(isset($this->data['countries']) ? $this->data['countries']->pluck('id')->all() : []),
                ],
                'address' => 'bail|string|required|alpha_numeric_seperated|regex:/\\d/|min:4|max:255',
                'zip_code' => 'bail|string|required|zip_code|max:10',
                'city' => 'bail|string|required|alpha_seperated|max:255',
                'email' => 'bail|string|required|email|max:255',
                'phone' => 'bail|string|required|phone|min:8|max:14',
                'contact_source' => [
                    'bail',
                    'numeric',
                    'string',
                    'required',
                    Rule::in($this->data['contactSources']->pluck('id')->all()),
                ],
                'education' => [
                    'bail',
                    'numeric',
                    'required',
                    Rule::in(isset($this->data['educations']) ? $this->data['educations']->pluck('id')->all() : []),
                ],

                // Not internship
                'drivers_license' => [
                    'bail',
                    'numeric',
                    Rule::in(
                        isset($this->data['driverLicenses']) ? $this->data['driverLicenses']->pluck('id')->all() : [],
                    ),
                ],
                'jobdeal' => [
                    'bail',
                    'required',
                    'numeric',
                    // add -1 to prevent required_if from triggering if query result is empty
                    'required_if:application.jobdeal,' . $this->data['jobdeals']
                        ->where('target', true)
                        ->pluck('id')
                        ->implode(',') . ',-1',
                    Rule::in(isset($this->data['jobdeals']) ? $this->data['jobdeals']->pluck('id')->all() : []),
                ],
                'civil_servant' => [
                    'bail',
                    'numeric',
                    'required',
                    Rule::in(
                        isset($this->data['civilServantOptions'])
                            ? $this->data['civilServantOptions']->pluck('id')->all()
                            : [],
                    ),
                ],
                // Only civil_servant
                'civil_servant_department' => [
                    'bail',
                    'numeric',
                    'required_if:application.civil_servant,' . $civilServantId,
                    Rule::in(
                        isset($this->data['civilServantDepartments'])
                            ? $this->data['civilServantDepartments']->pluck('id')->all()
                            : [],
                    ),
                ],
                'civil_servant_unit' => [
                    'bail',
                    'numeric',
                    // add -1 to prevent required_if from triggering if query result is empty
                    'required_if:application.civil_servant_department,' . $this->data['civilServantDepartments']
                        ->where('target', true)
                        ->pluck('id')
                        ->implode(',') . ',-1',
                    Rule::in(
                        isset($this->data['civilServantUnits'])
                            ? $this->data['civilServantUnits']->pluck('id')->all()
                            : [],
                    ),
                ],
                'civil_servant_contract' => [
                    'bail',
                    'numeric',
                    'required_if:application.civil_servant,' . $civilServantId,
                    Rule::in(
                        isset($this->data['civilServantContracts'])
                            ? $this->data['civilServantContracts']->pluck('id')->all()
                            : [],
                    ),
                ],
                'civil_servant_current_function_group' => [
                    'bail',
                    'string',
                    // add -1 to prevent required_if from triggering if query result is empty
                    'required_if:application.civil_servant_contract,' . $this->data['civilServantContracts']
                        ->where('target', true)
                        ->pluck('id')
                        ->implode(',') . ',-1',
                    Rule::in(
                        isset($this->data['civilServantCurrentFunctionGroups'])
                            ? $this->data['civilServantCurrentFunctionGroups']->pluck('id')->all()
                            : [],
                    ),
                ],
                'civil_servant_current_function' => [
                    'bail',
                    'numeric',
                    // add -1 to prevent required_if from triggering if query result is empty
                    'required_if:application.civil_servant_current_function_group,' . $this->data['civilServantCurrentFunctionGroups']
                        ->where('target', true)
                        ->pluck('id')
                        ->implode(',') . ',-1',
                    Rule::in(
                        is_string(
                            $this->request->input('application.civil_servant_current_function_group'),
                        ) && isset(
                            $this->data['civilServantCurrentFunctions'][$this->request->input(
                                'application.civil_servant_current_function_group',
                            )],
                        ) ?
                            $this->data['civilServantCurrentFunctions'][$this->request->input(
                                'application.civil_servant_current_function_group',
                            )]->pluck('id')->all()
                            : [],
                    ),
                ],
                'civil_servant_salary' => [
                    'bail',
                    // add -1 to prevent required_if from triggering if query result is empty
                    'required_if:application.civil_servant_current_function_group,' . $this->data['civilServantCurrentFunctionGroups']
                        ->pluck('id')
                        ->implode(',') . ',-1',
                    'numeric',
                    Rule::in(
                        isset($this->data['civilServantSalaries'])
                            ? $this->data['civilServantSalaries']->pluck('id')->all()
                            : [],
                    ),
                ],
                'civil_servant_sap' => [
                    'bail',
                    'numeric',
                    // add -1 to prevent required_if from triggering if query result is empty
                    'required_if:application.civil_servant_department,' . $this->data['civilServantDepartments']
                        ->where('target', true)
                        ->pluck('id')
                        ->implode(',') . ',-1',
                    'digits_between:4,8',
                ],
                'civil_servant_preferred' => [
                    'bail',
                    'numeric',
                    // add -1 to prevent required_if from triggering if query result is empty
                    'required_if:application.civil_servant_contract,' . $this->data['civilServantContracts']
                        ->where('fixed', true)
                        ->pluck('id')
                        ->implode(',') . ',-1',
                    Rule::in(
                        isset($this->data['civilServantPreferred'])
                            ? $this->data['civilServantPreferred']->pluck('id')->all()
                            : [],
                    ),
                ],

                // Only internship
                'internship_year' => [
                    'bail',
                    'integer',
                    'required',
                    Rule::in(
                        isset($this->data['internshipStudyYears'])
                            ? $this->data['internshipStudyYears']->pluck('id')->all()
                            : [],
                    ),
                ],
                'internship_school' => 'bail|string|required|alpha_extended',
                'internship_guide' => 'bail|string|alpha_extended',
                'internship_date_from' => [
                    'bail',
                    'string',
                    'required',
                    'date_format:d-m-Y',
                    'before:application.internship_date_to',
                    'after:' . date('d-m-Y'),
                ],
                'internship_date_to' => [
                    'bail',
                    'string',
                    'required',
                    'date_format:d-m-Y',
                    'after:' . date('d-m-Y'),
                ],
                'internship_hours' => 'bail|integer|required|between:2,80',
                'internship_type' => [
                    'bail',
                    'numeric',
                    'required',
                    Rule::in(
                        isset($this->data['internshipTypes'])
                            ? $this->data['internshipTypes']->pluck('id')->all()
                            : [],
                    ),
                ],

                'work_area' => [
                    'bail',
                    'numeric',
                    'required',
                    Rule::in(isset($this->data['workAreas']) ? $this->data['workAreas']->pluck('id')->all() : []),
                ],
                'experience' => [
                    'bail',
                    'numeric',
                    'required',
                    Rule::in(
                        isset($this->data['experiences'])
                            ? $this->data['experiences']->pluck('id')->all()
                            : [],
                    ),
                ],

                'note' => 'bail|string|max:2000|no_html',
                'upload_cv' => (!$this->request->session()->get('application.cv') ? 'required|'
                        : '') . $fileUploadValidationRule,
                'upload_motivation' => (!$this->request->session()->get('application.motivation') ? 'required|'
                        : '') . $fileUploadValidationRule,
                'upload_prio' => (!$this->request->session()->get('application.prio') ?
                        'required_if:application.civil_servant_preferred,' . $this->data['civilServantPreferred']
                            ->where('target', true)
                            ->pluck('id')
                            ->implode(',') . ',-1|' : '') . $fileUploadValidationRule,
                'upload_additional_1' => $fileUploadValidationRule,
                'privacy' => 'bail|boolean|required',
                'confirm' => 'bail|boolean|required',
                'research_agreement' => 'bail|boolean',
            ],
        ];

        $netherlandsId = $this->data['countries']->where('slug', 'nederland')->first()->id;
        if ((int)$this->request->input('application.country') !== $netherlandsId) {
            $result['application']['zip_code'] = str_replace(
                'zip_code',
                'alpha_numeric_spaces',
                $result['application']['zip_code'],
            );
        }

        return arrayFlat($result, '', true);
    }
}

Expanding the collapsed code has a bit of a weird behaviour. But I hope you get the gist of it :)

[sirbrillig]

Thanks! (I adjusted your comment to make the code a block.) Super helpful. It turns out that there's some pretty slow recursion going on in getListAssignments for nested list destructuring. I'll see if I can fix that.

phpcs-variable-analysis/VariableAnalysis/Lib/Helpers.php

Lines 607 to 609 in 5048655

	foreach (array_reverse($parents, true) as $openParen => $closeParen) {
	$nestedAssignments = self::getListAssignments($phpcsFile, $openParen);
	}

[Levivb]

ah, thanks. Was looking for that syntax but couldn't quite figure that one out :)

Great work on pinpointing the responsible code that fast 👍

[sirbrillig]

Great work on pinpointing the responsible code that fast

😁 xdebug profiling is magical.

[jrfnl]

@sirbrillig I presume you copied the underlying code for that from PHPCSUtils ? In that case, I know there is an issue with the isShortArray()/isShortList() functions. I'm working on a fix and it's one of the things which the alpha-4 release is waiting on.

Current status: I've been using BlackFire for the profiling and one particularly large array as a basis (the one through which I discovered the issue) - already managed to bring it down from 120 seconds to 15, but still feel it can probably be brought down further.

[sirbrillig]

already managed to bring it down from 120 seconds to 15, but still feel it can probably be brought down further.

Thanks for the info! I'm excited for when we can use this! 👏

I presume you copied the underlying code for that from PHPCSUtils

I think I ended up taking a different approach. I'm sure your approach is much better, but hopefully this will be good enough for the time being. Notably, I think your version does not use recursion, where mine does, and that was the cause of this performance issue. I adjusted the recursion a bit in #218 which should fix the problem.

[Levivb]

Running phpcs with 2.10.1 on the big file:

Time: 958ms; Memory: 40MB

Other files are around 140-240 ms, but this is still much better than the original 90 seconds 👍 😁

[andypost]

As PHP 8.2 beta1 will be released today (packaged already)
I did check current 2.x-dev#e9c99cd with Drupal 10.0-dev and all problems are gone!

+1 to tag a new 2.x release 2.11.4 or 2.12.0, please it will help to speed-up adoption

PS we running tests for PHP 7.3-8.2a3

[sirbrillig]

+1 to tag a new 2.x release 2.11.4 or 2.12.0, please it will help to speed-up adoption

🤔 To my knowledge, nothing has changed since 2.11.3 was tagged except for coding standards and formatting internal to the sniff. Here's all the changes since that tag. Was one of those commits fixing a problem with some environment?

[andypost]

@sirbrillig c3243c8 this commit fixes

PHP Deprecated:  Using ${var} in strings is deprecated, use {$var} instead in /var/www/html/web/vendor/sirbrillig/phpcs-variable-analysis/VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php on line 922

[sirbrillig]

Ah, good point. I've released 2.11.4 which now includes that commit.

https://github.com/sirbrillig/phpcs-variable-analysis/releases/tag/v2.11.4

[andypost]

Thank you a lot!