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