Fix PHPCS and maybe other errors

• PHPCS - fixed
• eslint - fixed; was complaining about an empty .yml value. Even though providing an empty string doesn't do much, I'm sure it's fine (given the change was done in response to a PHP error which is OK with an empty string - 🐛 First-load of gitpod demo often shows an error Fixed )
• PHPUnit - skipping. Complains about missing schema for custom_elements, needs to be fixed at 📌 Check and extend config schema Active
• PHPStan - partly fixed.

PHPStan issues:

1)
The fix requires a change to the constructor / service parameters of BaseUrlProvider. I have checked that this does not interfere with a cache clear, and I don't expect this class to be used in very low level code. (So basically, redirects will fail during an update until the cache clear - during which any site should be in maintenance mode.)

...that's fine, right?

2)
PHPStan complains about "Unsafe usage of new static()" in ViewsController and Drupal suggests a fix of →

• either making the class final
• or adding an exception to phpstan.neon.

I am leaning toward the second thing, because there is some kind of maybe useful/extensible code in the controller. But there's no phpstan.neon in this project yet. And I'm not the code owner.

Opinions?

phpcs doesn't report any issues:

$ composer cs web/modules/contrib/lupus_decoupled                               
> vendor/bin/phpcs --colors 'web/modules/contrib/lupus_decoupled'

neither does phpstan (with my local project config, level 0 sadly). I guess we could have a new issue to raise level to 3 or above.

$ composer phpstan-check web/modules/contrib/lupus_decoupled
> vendor/bin/phpstan analyze --memory-limit=-1 'web/modules/contrib/lupus_decoupled'
Note: Using configuration file /home/lio/Projects/drunomix/ldp/ldp-project/phpstan.neon.
 20/20 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%
                                                                                                                        
 [OK] No errors                                                                                                         
                                                                                                                        

[ Oh, I get why this issue is here now. It's because we've switched to Gitlab testing a month ago, and probably the test configuration flags more errors. ]

Well... the issue is, PHPStan does actually give errors here. See the red cross on this issue's MR above (now that there is one).

However...

• After digging into configs a bit, I see that the one reported issue is explicitly noted as ignoreErrors by Core... so I'm now confident we can ignore it too.
• Since I need to fix more errors in 📌 Add responsive preview functionality Needs work , I'll just add the whole phpstan.neon there, to prevent merge conflicts. That also means this can stay RTBC :-)

This weekend I dug into Gitlab CI → / PHPStan a bit.

Our PHPStan job (standard) definition is here. I cannot find out from it, or from the Gitlab CI job console output, how to run a local command with the exact same outcome / which exact configuration it uses. (I suspect the PHPSTAN_CONFIGURATION is empty. At any rate it does not include Core's phpstan.neon.dist; see my previous message.)

There aren't many examples of projects using their own phpstan.neon yet. (I could find zero.) But I've outlined my opinion on including one (vs. fixing the static() call).

After including the phpstan.neon, suddenly an extra error appeared (that was not visible in Gitlab CI yet (although it was visible for me locally with both level 0 and 1...) No clue why, but - fixed it. I guess we have a stable baseline now.

So...

• This is Needs Review for those changes / a definite (implicit or explicit) OK on adding phpstan.neon.
• We're still 'red' but there's only one reason: the PHPUnit error that should be fixed at 📌 Check and extend config schema Active .

When I actually looked, I found out that fixing PHPUnit isn't actually that hard. It's just a matter of adding configuration schema to various modules, including this one.

Unfortunately, PHPUnit will only stop failing after 📌 Check and extend config schema Active and 📌 Missing config schema Needs review are done. (And then either new releases should be made or our composer.json should require-dev the -dev versions... I guess.)

Thanks for amazing contribution. MR looks good.
Can we disable this phpunit test that is not able to pass, until issues will be fixed?

I'm new to PHPStan and for some reason have difficulty (which I'm not going to investigate right now) running it locally without getting loads of other errors.

So... trying level 2 on Gitlab CI and seeing what fails.

Anyway I should have remembered to keep this at "Needs work" assuming 📌 Dependency injection anti-patterns Needs work gets in first, and then rebase after it.

I am also trying to get more familiar with PHPStan in Drupal slack there is channel called #phpstan, you can find support about this tool.
I see there are 3 errors in the pipeline, you can add them to phpstan-baseline.neon (from artifacts) file and them include this file in the phpstan.neon. Example.

We could add exceptions to phpstan.neon, but... now that I looked at those code lines, I think PHPStan is right and we should fix things.

Above commit / MR comment is for review. (camelCase vars now fixed too.)

I've ran phpstan w/ level 2 locally and result was green.
phpcs result was also green.

Code changes looks good to me. PR probably still needs a re-roll bcs 📌 Dependency injection anti-patterns Needs work got merged.

Rebased; verified diffs before & after; did a quick test to see that the constructor indeed does what it's supposed to.

Then fixed indentation from the just-merged issue.

If PHPStan level 3 is only that, then let's fix it - it's right. See last pushed commit: it was just a fix in a variable-comment (I hope).

PHPUnit still failing, but that wants a custom_elements release. (I guess we could decide to try and require-dev the dev version, or wait for a new release.)

Code changes are good.
I propose we merge now.

merged

Automatically closed - issue fixed for 2 weeks with no activity.