Increase phpstan level to max and implement constructor property promotion

[andyexeter]

This PR increases the phpstan level to max and fixes all associated errors at this level. I have tried to group each set of fixes into individual commits which I hope makes it easier to review. Let me know if you want me to squash the commits into one.

The main fixes were:

• Adding missing types
• Fixing incorrect types
• Removing dead code (methods, arguments, variables and properties)

I also replaced uses of strpos and substr with str_starts_with and str_ends_with where applicable - these weren't being reported as phpstan errors but I thought it made sense to include them in this PR as a general refactoring.

The errors ignored in phpstan.neon are related to value types not being defined on arrays (https://phpstan.org/blog/solving-phpstan-no-value-type-specified-in-iterable-type). To fix these, this would require going through the 52 instances where arrays are defined in the code base and adding value (and possibly key) types, e.g.:

/** @var array<string, string> $someArray */

I don't have the time to do this at the moment but it could be looked at afterwards if this PR gets merged.

I also wanted to refactor the classes into using constructor property promotion but I wanted to get your thoughts on this first @lhapaipai - would you be happy for me to do this? It would deduplicate a lot of code, for example the constructor in TagRenderer:

private array $globalDefaultAttributes;
private array $globalScriptAttributes;
private array $globalLinkAttributes;
private array $globalPreloadAttributes;

public function __construct(
    array $defaultAttributes = [],
    array $scriptAttributes = [],
    array $linkAttributes = [],
    array $preloadAttributes = []
) {
    $this->globalDefaultAttributes = $defaultAttributes;
    $this->globalScriptAttributes = $scriptAttributes;
    $this->globalLinkAttributes = $linkAttributes;
    $this->globalPreloadAttributes = $preloadAttributes;
}

Would become:

public function __construct(
    private array $globalDefaultAttributes = [],
    private array $globalScriptAttributes = [],
    private array $globalLinkAttributes = [],
    private array $globalPreloadAttributes = []
) {
}

[netlify[bot]]

✅ Deploy Preview for cosmic-bubblegum-aa631a canceled.

Name	Link
🔨 Latest commit	b1b864a59cccebc05760d0073457f1d1d7b78773
🔍 Latest deploy log	https://app.netlify.com/sites/cosmic-bubblegum-aa631a/deploys/65cf28985d93960008a9d968

[lhapaipai]

hey @andyexeter, of course, I totally approve of this work.

regarding the possible refactoring with php8.0 the same I completely agree. I was planning to do just that because it's true that the constructors were very verbose...

regarding commits, I am in favor of making small commits. in this case I don't think you needed so much detail as long as you leave a comment in your pr. but for this case leave it as is !

good afternoon.

[andyexeter]

CI was failing on PHP 8.0 because the workflow is installing Symfony 6.0 (the highest supported version which will run on PHP 8.0) which doesn't contain the required template notation (https://github.com/symfony/symfony/pull/49022)

I have worked around this by ignoring these errors just for that PHP version.

[andyexeter]

@lhapaipai I've implemented constructor property promotion and all checks are green so this is ready for review :)

[lhapaipai]

Hi @andyexeter, thank you for your work. I have never gone into so much detail with PHPDoc / PHPstan and I find it very interesting...

so I wanted to type remaining arrays and I admit that I don't really know how to go about it when I have to type content coming from json/yaml....

commit

I easily find myself doing array<mixed>, array<string, mixed>. suddenly we end up with half-typed content, so we can half-trust and that doesn't quite suit me. and PHPStan also didn't convince me see.

namespace Pentatrion\ViteBundle\Service;

use Pentatrion\ViteBundle\Exception\EntrypointNotFoundException;

class EntrypointsLookup
{
    /** @var array<mixed>|null */
    private ?array $fileContent = null;

    public function __construct(
        private FileAccessor $fileAccessor,
        private string $configName,
        // for cache to retrieve content : configName is cache key
        private bool $throwOnMissingEntry = false
    ) {
    }

    /**
     * @return array<mixed>
     */
    private function getFileContent(): array
    {
        if (is_null($this->fileContent)) {
            $this->fileContent = $this->fileAccessor->getData($this->configName, 'entrypoints');

            if (!array_key_exists('entryPoints', $this->fileContent)
            || !array_key_exists('viteServer', $this->fileContent)
            || !array_key_exists('base', $this->fileContent)
            ) {
                throw new \Exception("$this->configName entrypoints.json : entryPoints, base or viteServer not exists");
            }
        }

        return $this->fileContent;
    }

    public function getFileHash(string $filePath): ?string
    {
        $infos = $this->getFileContent();

-       // Parameter #2 $array of function array_key_exists expects array, mixed given.PHPStan
        if (is_null($infos['metadatas']) || !array_key_exists($filePath, $infos['metadatas'])) {
            return null;
        }

        return $infos['metadatas'][$filePath]['hash'];
    }

    // ...
}

what good practice do you recommend rather than defining mixed content, to define a shape with an interface/type as we could do in TypeScript ?

/** @var array<mixed>|null */
private ?array $fileContent = null;

Finally, in order to facilitate your development, I would like to give you a right as a collaborator on the main repository, would you agree?

[andyexeter]

Hi @lhapaipai,

To define types in phpstan you can use the @phpstan-type annotation (https://phpstan.org/writing-php-code/phpdoc-types#local-type-aliases), e.g:

/**
 * @phpstan-type EntrypointsJsonEntrypoint array<string, list<string>>
 * @phpstan-type EntrypointsJson array{base: string, viteServer: string, legacy: boolean, entryPoints: array<string, EntrypointsJsonEntrypoint>}
 */

Then you can use the types like:

/**
 * @phpstan-return EntrypointsJson
 */
private function getFileContent(): array
{

I wrote a quick example on phpstan.org: https://phpstan.org/r/25485c00-6f2f-4e86-ba57-cf4bb156c439

Yes I am happy to be given access as a collaborator, thank you!

[lhapaipai]

Hi @andyexeter , I added types for array values from json/yaml but I've still have one error in the src/vite-bundle/src/Service/FileAccessor.php file getData method.

I've also one error by my Intelephense extension (vscode) : src/vite-bundle/src/DependencyInjection/PentatrionViteExtension.php

I would like to know what configuration you have for your php lint ?

for my part I am on vscode with the PHP Intelephense and PHPstan extensions (which executes phpstan in the background). and I'm a little troubled because I have the impression that these 2 linters don't have the same understanding of the code. should I deactivate one?

thank you for your response

I gave you access to the main repository perhaps you could edit this PR so that it points to the branch: andyexeter-phpstan-level-increase? I don't know if it's possible to edit the origin?

[andyexeter]

Thanks @lhapaipai, I've accepted the invitation.

I use PhpStorm so I don't think I can help with the vscode issues. In PhpStorm I don't see any errors about the $configPrepared array. Also, if I try to reference a non-existing key I get an error so it does seem to know the array shape:

I can't change the origin branch for the PR but I have rebased so this PR contains your commits from the other branch.

I've also fixed the remaining error in the FIleAccessor getData method: a782bdc

[lhapaipai]

great ! you can merge.

for the rest I trust you completely so you can apply patches without even asking my opinion.

if it involves adding/modifying minor features, however, we consult before starting to code.

One thing I care about: keeping the documentation up to date If you want to add content to the doc, can you also put your addition on the French version (google translate or even in English if you are not comfortable with the language). it's easier to see afterwards what remains to be translated.

I don't know if you saw but I automated the version change bin/change-version.sh.

• it adds the git tags
• changes in the code the places where the version number is hardcoded
• pushes the code into the 2 read-only repositories
• publish the vite plugin on npm.

you won't be able to do it alone because you don't have npm access.

so if you need a version change, just ask me so I can run the script.

Finally, if you have any questions, don't hesitate !!

Good afternoon

[andyexeter]

Thanks! I do have one question - when merging PRs do you prefer a "Squash and merge" or a regular merge?

[lhapaipai]

hi @andyexeter, I prefer a regular merge. I don't mind having a not very pretty git tree but I prefer to preserve the original commits. Of course, this can also be discussed if it doesn't suit you.