Problem/Motivation

The tests are failing on the 4.x branch, likely because of some change in the latest Drupal version.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Issue fork simplenews-3542503

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:

Comments

adamps created an issue. See original summary.

adamps’s picture

There is one easy fix: add static to public function simplenewsNewsletterNameTokenDataProvider() in 2 places.

The other failures relate to the simplenews_issue field being missing, which is strange. It should come from /config/optional/field.field.node.simplenews_issue.simplenews_issue.yml.

davps made their first commit to this issue’s fork.

davps’s picture

The other failures relate to the simplenews_issue field being missing, which is strange

@adamps, the problem is relevant only for core version 11.2.* and higher - for previous versions everything is fine.

The module installation process has changed its default behavior since core version 11.2.0:

I updated the configuration file and added testing on previous core versions and you can see (https://git.drupalcode.org/issue/simplenews-3542503/-/pipelines/650213) that the problem appeared only in the latest minor version:

  • phpunit (previous major) job: Drupal installed version - 10.5.4
  • phpunit (previous minor) job: Drupal installed version - 11.1.8
  • phpunit job: Drupal installed version - 11.2.5

I see the following solutions from my side:

  1. decide that node-related configuration files are mandatory and move them to config/install directory
  2. rethink and refactor a method "simplenews_demo_install" that expects "optional" changes related to node
  3. specify parameter "container_rebuild_required" for the simplenews_demo module
davps’s picture

Status: Active » Needs review

@adamps, based on the comment above, I suggest the simplest and quickest solution – specify parameter "container_rebuild_required" for the simplenews_demo module.

I also suggest keeping the testing rules for previous core versions. It may be necessary to update the rules over time to maintain their relevance.

schillerm’s picture

Hi, I was getting errors when running PHPUnit tests on !MR 95 .. but realised I was missing token and monitoring modules. Re ran tests .. no errors now.

adamps’s picture

Status: Needs review » Fixed

Great thanks

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.

adamps’s picture

Status: Fixed » Needs work

Unfortunately there is now another test failure

loze’s picture

Root cause of the functional failures on 11.3+ (verified locally on 11.4):

The testing profile now ships field.storage.node.body as text_long (#3477043: Change automatic body field creation to use formatted text field instead of text_with_summary), but simplenews ships simplenews_issue's body as text_with_summary. A field instance's type is inherited from its storage, so every attempt to create a simplenews_issue node throws InvalidArgumentException: Property summary is unknown (the text_long storage has no summary). That is what breaks SimplenewsSendTest, SimplenewsSubscribeTest, SimplenewsAdministrationTest, and the rest. Real sites on the standard profile are unaffected (their body storage is still text_with_summary); it only hits the test environment.

Two obvious fixes do NOT work, both because the profile installs text_long first and wins:

  • Enabling core's node_storage_body_field in the test modules.
  • Shipping a text_with_summary field.storage.node.body from simplenews_test (the views_advanced_cache #3571415: Tests failing with Drupal 11.3.2 fix; their case was a missing storage, ours is a wrong-typed one).

Since simplenews reuses the shared body storage, whose type it does not control and which now varies by site, this needs a decision rather than a config tweak:

  • Keep text_with_summary and depend on node_storage_body_field. No data migration. Downside: that module is deprecated, and it still loses to the testing profile, so the tests need a separate workaround anyway.
  • Switch simplenews_issue's body to text_long. Matches core, and the summary is already unused (display_summary: false). Downside: existing installs have a text_with_summary body, so this needs an update hook and data migration, not just a config change.
  • Give simplenews_issue its own dedicated body field instead of reusing shared body. Fully robust and immune to the profile. Downside: the biggest change, a new field plus migrating existing content into it.

Happy to build whichever direction you want.

loze’s picture

Status: Needs work » Needs review
loze’s picture

After thinking on this a little more I realized there is a fourth option, which avoids the downsides of all three above. I think this is the best approach. I have pushed it to !95.

The fix. Stop shipping the body field instance as static YAML and create it in code at install time, inheriting the type of whatever body storage the site has (creating a text_long storage if none exists). A field created from the storage object cannot mismatch it, so the bug becomes structurally impossible on every profile and every core version. It runs from hook_modules_installed(), after the optional config (content type and displays) has been imported.

Why it is safe. Existing installs are completely untouched, because optional config and install hooks only run at install time. No update hook, no data migration, and a site that enabled the summary on its body field keeps it, along with its data. Only new installs are affected, and they get a body field that matches their site.

Precedent. This is the same pattern as core's node_add_body_field() (made storage-type-aware, which is why that function was deprecated) and media source fields, and simplenews's own newsletter checkbox already creates the issue field this way.

Getting the suite fully green on 11.4 also surfaced a few real bugs, fixed in the same MR:

  • SpoolStorage cached the config object at construction, so configuration changed later in the process (like mail.spool_expire) was read stale. It now stores the factory and reads at use time.
  • The "Add Newsletter Issue" action link is derived from the newsletter content types, but its plugin cache was never cleared when a type was added or removed, leaving a stale link. The clear is intentionally unconditional: at deletion time the bundle's fields are being cleaned up in the same operation, so checking "was this a newsletter type" cannot be trusted, and type deletion is rare enough that an extra rebuild costs nothing.
  • The display-settings help text was missing on 11.x because Field UI's overview page is a new route (entity.entity_view_display_overview.node).

One deliberate behavior addition, flag it if you disagree: since core no longer adds a body field to new content types (11.2), marking a content type as a newsletter now gives it one too, through the same helper. Existing body fields are never touched. That restores how newsletter content types worked before 11.2, when core provided the body.

Coverage for all of this: a kernel test for each body-storage scenario (text_long, text_with_summary, missing, repurposed, rerun), a functional test that installing the module produces the field and displays, and an assertion that the newsletter checkbox adds the body. The monitoring kernel test also needed the filter module enabled (the subscribers view requires it on 11.x).

loze’s picture

Status: Needs review » Needs work
loze’s picture

Assigned: Unassigned » loze
loze’s picture

The pipeline is green now: phpunit passes on the current core, with the whole functional suite, the kernel tests, and the new coverage running. I have also run the full functional suite locally on 10.6 and 11.4, both pass.

The only jobs still failing are the ones allowed to fail, and none of the findings are from this MR:

  • phpcs: 12 errors / 12 warnings across the module, 7 of them phpcbf-autofixable, the rest missing doc comments.
  • phpstan: mostly "\Drupal calls should be avoided, use dependency injection" refactors, plus calls to functions deprecated in 11.2+ whose replacements do not exist on Drupal 10, so those cannot be fixed while 4.x supports ^10.
  • cspell: a handful of words like snid, msid and one British spelling, dictionary or one-word fixes.
  • eslint: pre-existing, no JavaScript is touched here.

Happy to fix any of those here if you prefer, but they are unrelated to the test failures, so I would keep this MR as is and handle them in follow-up issues.

I think this is ready to commit.

loze’s picture

Assigned: loze » Unassigned
Status: Needs work » Needs review

  • adamps committed 6d9f2ddb on 4.x authored by davps
    task: #3542503 Fix tests
    
    By: adamps
    By: davps
    By: loze
    
adamps’s picture

Status: Needs review » Fixed

Looks good to me many thanks

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.