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
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
Comment #2
adamps commentedThere is one easy fix: add
statictopublic function simplenewsNewsletterNameTokenDataProvider()in 2 places.The other failures relate to the
simplenews_issuefield being missing, which is strange. It should come from/config/optional/field.field.node.simplenews_issue.simplenews_issue.yml.Comment #5
davps commented@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:
I see the following solutions from my side:
Comment #6
davps commented@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.
Comment #7
schillerm commentedHi, 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.
Comment #8
adamps commentedGreat thanks
Comment #10
adamps commentedUnfortunately there is now another test failure
Comment #11
loze commentedRoot cause of the functional failures on 11.3+ (verified locally on 11.4):
The
testingprofile now shipsfield.storage.node.bodyastext_long(#3477043: Change automatic body field creation to use formatted text field instead of text_with_summary), but simplenews shipssimplenews_issue's body astext_with_summary. A field instance's type is inherited from its storage, so every attempt to create asimplenews_issuenode throwsInvalidArgumentException: Property summary is unknown(thetext_longstorage has nosummary). That is what breaks SimplenewsSendTest, SimplenewsSubscribeTest, SimplenewsAdministrationTest, and the rest. Real sites on the standard profile are unaffected (their body storage is stilltext_with_summary); it only hits the test environment.Two obvious fixes do NOT work, both because the profile installs
text_longfirst and wins:node_storage_body_fieldin the test modules.text_with_summaryfield.storage.node.bodyfromsimplenews_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
bodystorage, whose type it does not control and which now varies by site, this needs a decision rather than a config tweak:text_with_summaryand depend onnode_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.simplenews_issue's body totext_long. Matches core, and the summary is already unused (display_summary: false). Downside: existing installs have atext_with_summarybody, so this needs an update hook and data migration, not just a config change.simplenews_issueits own dedicated body field instead of reusing sharedbody. 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.
Comment #12
loze commentedComment #13
loze commentedAfter 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
bodystorage the site has (creating atext_longstorage 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 fromhook_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:
SpoolStoragecached the config object at construction, so configuration changed later in the process (likemail.spool_expire) was read stale. It now stores the factory and reads at use time.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).
Comment #14
loze commentedComment #15
loze commentedComment #16
loze commentedThe 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:
snid,msidand one British spelling, dictionary or one-word fixes.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.
Comment #17
loze commentedComment #19
adamps commentedLooks good to me many thanks