phpstan jobs fails at GitLab

Here are the current `phpstan` job results ([ERROR] Found 43 errors):
https://git.drupalcode.org/project/symfony_mailer/-/jobs/322855

The Drupal core `phpstan` configuration has been applied:
https://git.drupalcode.org/project/drupal/-/blob/10.0.x/core/phpstan.neo...

Here are the current results ([ERROR] Found 8 errors):
https://git.drupalcode.org/issue/symfony_mailer-3401293/-/jobs/323458

@Adam, could you please review the current changes and commit them if everything looks okay?
Then, we will process with the rest (to avoid huge MRs).

____

The rest of the errors are related to using methods/classes from 3rd party modules (like simplenews, commerce, etc).
I assume the best way is to have submodules (with the appropriate dependencies) like symfony_mailer_simplenews, symfony_mailer_commerce and move everything related to those modules into them. In this case, we will be able better to control the dependencies / tests / phpstan errors.
@Adam, what do you think?

Thanks!

Thanks for your enthusiasm to improve this module. I see phpstan in the same category as grammar checkers in word processors. Sometimes it can help, but also it can have a lot of wrong ideas😃.

I see a mixture of:

• real bugs - let's fix them
• not actually bugs, but they look a bit like bugs: as a human I can understand that in fact the for loop must always be executed. For example, the fix to TransportAddButtonForm doesn't really make sense, because if $options actually could be empty, then you would have a select with no possible values which would be incorrect. Fortunately there are always options as this module defines some transports, so therefore the code is fine as it is.
• just pedantic and not really important: isset or empty
• making the code worse: most of the changes to constants. What we actually want is like this question. The base class should not have a value because the child should be forced to define one. 'To' is not correct as the default for an address.
• the tool seems wrong: in Core the @param sometimes doesn't have a question mark when there is = NULL
• the tool is wrong: SWIFTMAILER_TRANSPORT_SMTP is fine because it is only used inside a test that the module exists
• the tool is wrong: surely the whole point of ?? is for something that might not be defined?

I assume the best way is to have submodules (with the appropriate dependencies) like symfony_mailer_simplenews, symfony_mailer_commerce and move everything related to those modules into them

This would add quite a lot of complexity and be difficult for back-compatibility of existing sites. What is the benefit - it seems only to cover limitations in a tool, and the tool anyway isn't that important. It seems like a clear no to me.

It feels to me that we should fix the missing use statements, and probably not most of the rest.

I am willing to compromise. In some cases although the change to code isn't necessary also it is harmless, so we could change it. However I don't agree to make the code worse or wrong just because the tool doesn't understand - maybe we could add a comment to disable the check of the specific line?? If you are interested then let's discuss it.

I committed the 2 use statements which are of course real bugs - thanks😃

Thanks @bohart . I found a compromise that avoids making the code worse yet removes the warnings.

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