Problem/Motivation

Currently the simplenews codbase is set to report when DI hasn't been followed as a best practice. While this could be a goal for the future, we want to avoid people feeling obligated to make the CI happy by fixing these changes. This is partially because enabling DI created Backwards Incompatible changes and can break contributed and custom code that are based on the current constructor signatures (see #3613461: How to handle DI and affects to code (backwards incompatible breaking changes)).

Steps to reproduce

Proposed resolution

For now, have PHPStan not suggest DI upgrades.

Remaining tasks

User interface changes

API changes

Data model changes

Issue fork simplenews-3613483

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

generalredneck created an issue. See original summary.

generalredneck opened merge request !113

generalredneck’s picture

generalredneck
him/his
Primary language English
Location Texas, USA 🇺🇸
commented
Parent issue: » #3613221: Pipelines warn about coding standards
generalredneck’s picture

generalredneck
him/his
Primary language English
Location Texas, USA 🇺🇸
commented
Status: Active » Needs review

This leaves Deprecations and some other issues. I think we need to "know" about the deprecations, but that's going to be it's own task... and those may already be handled in the D12 upgrade issue.

The other issues may need to be handled in another issue. even.

adamps’s picture

adamps commented
Status: Needs review » Needs work

Great thanks.

Please could we use identifiers? It seems more precise rather than testing an error message. Here's what I had in another project:

    # new static() is a best practice in Drupal, so we cannot fix that.
    # @see https://www.drupal.org/docs/develop/development-tools/phpstan/handling-unsafe-usage-of-new-static
    - identifier: new.static

    # Dependency injection leads to huge amounts of dull boiler plate code,
    # and BC problems so we don't always do it.
    - identifier: globalDrupalDependencyInjection.useDependencyInjection

I've just accepted the simplenews.drush.inc one so we could remove that already

generalredneck’s picture

generalredneck
him/his
Primary language English
Location Texas, USA 🇺🇸
commented

Oh sweet. I remember you asking for that and then I forgot to look up how to do it. I'll get on that when I get a few later today.

generalredneck’s picture

generalredneck
him/his
Primary language English
Location Texas, USA 🇺🇸
commented
Status: Needs work » Needs review

Rebased and swapped over to your suggested markup. I hope you don't mind that I totally ripped the example word for word.

adamps’s picture

adamps commented
Title: Remove DI suggestions for now to discourage massive patch changes » Disable phpstan warnings that aren't suitable for this module

adamps’s picture

adamps commented
Status: Needs review » Fixed

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

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