Pipelines warn about coding standards

I'm taking a first stab at this but it's a BIG change.

In light of what you said in your last comment, I made this a "draft" I feel like a lot of the work is valid and will be able to be rebased. I didn't want to lose it. Marking this as needs work for after a release. Here are my notes working through the codebase.

PHPStan & DI

Dependency injection (the bulk of it): refactored ~25 classes to inject services instead of calling \Drupal::… recipient handler plugins, forms (Confirm*, RequestHash, SubscriptionsBlock, etc.), controllers, actions, the views field/mail plugins, and the constraint validator. The trickiest part was Mailer/SpoolStorage/MailEntity/SpoolList, which all built MailEntity objects by hand with \Drupal::service() calls. I introduced a MailEntityFactory service to hold those dependencies centrally, and tried to break a circular dependency between (SpoolStorage ↔ Mailer) using setter injection (setMailer()) registered via a service calls: entry.

Bugs fixed while refactoring:

SubscriptionSettingsForm: removed a leftover duplicate #default_value key and reference to an undefined $database variable.
NewsletterForm::save() and Mailer::sendSpool()/sendSubscribeConfirmation(): added missing return statements/values per their declared contracts.
SubscriberHistory::t(): added StringTranslationTrait (the method didn't exist).
simplenews_entity_operation(): updated for Drupal 11.3's new CacheableMetadata parameter.
SimplenewsTestSubscriptionOutput.php was renamed to SimplenewsSubscriptionOutputTest.php: the old filename didn't match its class name pattern, so PHPUnit wasn't even discovering this test.
Fixed several class.nameCase typos (recipientHandler became RecipientHandler).
Deprecated API replacements: user_load_by_mail/user_load_by_name, user_pass_reset_url, user_pass_rehash, check_markup, field_purge_batch, filter_fallback_format, views_field_default_views_data, Renderer::renderPlain().

simplenews.drush.inc (5 errors): legacy Drush 8 functions. Going to tell PHPStan to ignore it.
Three D7 migrate source classes (6 errors): extend DrupalSqlBase, which core deprecated with no replacement. I'm going to tell PHPStan to ignore it too. We can decide if we want to keep these after a Drupal 12 release.

Fixing Monitor + ensuring tests run

Monitoring integration fixed: PendingSensorPlugin now uses the monitoring module's own $this->getService() convention instead of \Drupal::service(). Validated with SimplenewsMonitoringTest (passes) and confirmed clean in phpstan.

The SpoolStorage to Mailer circular-dependency fix I made earlier was itself broken. I'd assumed Drupal's DI container could resolve a constructor/setter circular reference the way a compiled Symfony ContainerBuilder can. It can't… Drupal ships its own lightweight runtime container that throws ServiceCircularReferenceException whenever neither side happens to already be cached. This silently killed cron-based mail sending (caught and logged by Drupal's per-hook cron exception handler, so it failed quietly rather than crashing). I reverted that one call site to the original \Drupal::service('simplenews.mailer'), documented in a comment why it's a legitimate, unavoidable exception, and confirmed via a debug trace that this really is the only way to break the cycle given Drupal's container implementation.

I also fixed a regression in SimplenewsSubscriberBulkFormTest.php where removing the flagged $this->storage property left two other usages in the same test dangling.

Verified via phpunit tests: SimplenewsMonitoringTest, the full simplenews Kernel suite (43 tests), SimplenewsAdministrationTest + SimplenewsPersonalizationFormsTest (13 tests), SimplenewsSubscribeTest (13 tests), and the bulk form test. All are green now, zero failures (only pre-existing deprecation noise from annotation-based plugin discovery, unrelated to this work). phpstan is at 12 remaining findings, all genuinely external (Drush 8 legacy functions, D7 migrate source deprecation with no replacement) or a documented, unavoidable DI exception.

Made PHPStan ignore the drush & migrate errors. Added an ignore line via code to the one place where DI is not possible, documenting in the code why.

PHPCS

adamps made a comment about how there are some rules that seem petty. It seems there was already a start to excluding rules before, so I added a couple. Specifically in the array declaration and long comment areas.

Ignored the "Possible useless method overriding detected" in the multilingual tests.

CSpell

Added our own .cspell.json. Since several of the leading developers are from Europe, it only makes sense that we add the en-gb as correct spelling. I learned a lot today about the differences in some words. That said, it's not exactly proper localization… Some of the messages that print out to the user are in en-gb… but I'm not going to gripe when UK natives have to put up with en-us BS on the regular. 😅 just pointing it out.
I didn't add Spanish as a language because there are only a few words in tests and I expect that we don't necessarily want the code to read in Spanish.

ESLint

3 optional configs have empty "content:" keys. We are just going to add { } to the mix to make ESLint be quiet. This should be functionally the same when imported. No need for an entity update either. It will be fixed if anyone imported that configuration and then hit save on any of the display modes and then they exported. Again they behave identically.

I think the the way forward is to remove the DI based standards then because even though your code ase is set to check for them you have stated no interest in moving toward the fairly abstract benefit. Following is a detailed reasoning for this thought.

I see your point on the replacing a class in custom modules and with 23K installs (per the project page) that's a possibility. Maybe we consider this on a new version if you ever have any interest in going that direction. Specifically maybe when you do finally remove old code

The challenge here is you're going to get people who are trying to contribute and follow the rules that the Drupal community has set forth and you have continuous integration that is enforcing those. They are trying to make that continuous integration. Happy as to get a better chance of getting their code in and or make the code better where they can.

Instead, your continuous integration is currently causing several of the issues on this project to be postponed because people then have to go back and rework their solutions.

You did mention that ignoring some of the rules specifically comment lines was something you were interested in. You also mentioned that di causes some challenges, but you did not suggest ignoring those rules.

My question to you is how would you like to proceed going forward in this particular portion given your strong messaging around stability.

I'm willing to do the work and several of your thoughts above have alternative ways forward, but that one major sticking point that is affecting several of the issues is a challenge we need to address so that you're not doing this all by yourself or ignoring the module as a whole because people can't contribute in the correct manner.

Adding related tasks to break this up.

I think that we can handle a few of these really easily. I need to work through your feedback on them but it shouldn't take too long. It will at least get us closer.

@adamps,

I know it's a lot of little things for review now. I hope it's easier. I honestly think that if we review them in this order:

1. #3613453: Fix Spelling issues defined by the pipeline
2. #3613455: Fix or ignore rules for PHPCS and ESLint checks
3. #3613474: Retire drush.inc command file
4. #3613483: Disable phpstan warnings that aren't suitable for this module
5. #3613485: Fix Legit PHPStan issues

It will make it easiest. Then from there if I have time in the future I can look at the rest of the stuff moving forward.

I've got to get back to the my farm as I make contribution time in between harvesting, managing birds, and running the business. Also I worked 20 years in Web Development... most of it in Drupal Agencies, so I get the "I only have X hours of maintainer time." I too am maintainer of many many modules... just none of them this size. Was trying to make some good use of it's too hot to be outside time.

Yeah... The main one that concerns me is this one

------ ---------------------------------------------------------------------- 
  Line   simplenews.module                                                     
 ------ ---------------------------------------------------------------------- 
  185    Function simplenews_entity_operation() implements                     
         hook_entity_operation but is missing the CacheableMetadata parameter  
         added in Drupal 11.3. Update the signature to:                        
         simplenews_entity_operation(\Drupal\Core\Entity\EntityInterface       
         $entity, \Drupal\Core\Cache\CacheableMetadata $cacheability).         
         🪪  drupal.proceduralHookEntityOperationMissingCacheabilityParameter  
         💡  See https://www.drupal.org/node/3533080

I'm assuming we need to allow $cacheablity to be NULL by default and that should make it work for older versions right? Guess I should make this it's own ticket.

The d12 ones should be fixed by a project bot ticket unless we need to creat a new one for that... And then lastly the ignore d13 deprecations.

Hey there,
Sorry I was sick yesterday and today I'm making up for it and also got a 6 hour drive to go get feed. I'm going to have to table this until this weekend unless I get a few minutes. Let me know how you would like to handle this l. I personally like the version bump as traditionally that's how modules handle major core upgrades.