Title: Add Drupal 12 support
Problem/Motivation
3.0.x declares ^10.3 || ^11.0. There is no automated
compatibility issue for this project, so nothing has flagged it.
Four separate things are in the way, and only the first is the sort of change
a version bump implies.
1. The token hooks throw
Drupal 12 no longer loads MODULE.tokens.inc before invoking the
hook. It registers dfp_tokens from the file scan and then cannot
resolve it:
InvalidArgumentException: Class "dfp_tokens" does not exist. in Drupal\Core\DependencyInjection\ClassResolver->getInstanceFromDefinition()
This is not only a test failure. It is any request that replaces a token, once
dfp is installed. Core made the same move and now ships
no .tokens.inc files at all — every one became a
*TokensHooks class, for example NodeTokensHooks.php.
2. HtmlResponseAttachmentsProcessor gained an argument
Constructing HtmlResponseAttachmentsProcessor without a file url generator is deprecated in drupal:11.4.0 and the argument will be required in drupal:12.0.0
DfpHtmlResponseAttachmentsProcessor subclasses it and does not
pass one, so the parent falls back to \Drupal::service(). In the
unit tests, where there is no container, that is a
ContainerNotInitializedException.
3. The test suite is not discovered
MissingGroupException: Missing group metadata in test Drupal\Tests\dfp\Functional\AlterTagTest::testDfpTagAltering
Drupal 12 reads the PHPUnit Group attribute rather than the
@group annotation, so nothing runs at all. Separately, the
@dataProvider annotations in TagViewTest are ignored
by PHPUnit 12, which fails the four tests that use them with
ArgumentCountError.
4. submitForm() and TranslatableMarkup
TypeError: WebAssert::buttonExists(): Argument #1 ($button) must be of type string|int|float|bool, Drupal\Core\StringTranslation\TranslatableMarkup given
Proposed resolution
-
Move the token hooks into
src/Hook/DfpTokensHooks.php, keeping
the procedural functions behindLegacyHookso 10.3 and 11 are
unaffected. -
Pass the file URL generator through to the parent constructor, and add it to
the service definition and the unit test. Passing the extra argument is
harmless on older core — PHP ignores it. -
Add the
Groupattribute to all five test classes and
RunTestsInSeparateProcessesto the functional ones. Add
DataProviderattributes alongside the existing
annotations rather than replacing them: PHPUnit 9 on Drupal 10 reads only
the annotation, PHPUnit 12 only the attribute. Dropping the annotation
breaks Drupal 10. - Pass a plain string as the submit button label.
-
core_version_requirement: ^10.3 || ^11 || ^12, in
dfp.info.ymland the test module.
Remaining tasks
Review. 27 of 27 tests pass on 10.6.15, 11.4.5 and 12.0-dev with the above.
The phpcs and phpstan findings that remain — two "String concat is not
required" in the test files, and some new static() and entity
storage warnings — all predate this and are untouched.
Worth noting separately: the repository's default branch is
8.x-1.x, while releases come from 3.0.x. Anyone
cloning lands on the wrong branch.
Comments
Comment #4
marcelovani>MR !46 >
is ready. Could another maintainer review? I would rather not merge my own
work here.
Four separate things were in the way, and only the last is the sort of change
a version bump implies.
The token hooks throw
Drupal 12 no longer loads
MODULE.tokens.incbefore invoking thehook. It registers
dfp_tokensfrom the file scan and then cannotresolve it:
This is not a test failure — it is any request that replaces a token, once dfp
is installed. Core made the same move and ships no
.tokens.incfiles at all now, every one having become a*TokensHooksclass. The implementations move tosrc/Hook/DfpTokensHooks.phpand the procedural functions staybehind
LegacyHook, so 10.3 and 11 are unaffected.The parent constructor gained an argument
HtmlResponseAttachmentsProcessor::__construct()takes a file URLgenerator, required from 12 and deprecated without since 11.4.
DfpHtmlResponseAttachmentsProcessorsubclasses it and did notpass one, so the unit tests died with
ContainerNotInitializedException. Passing the extra argument isharmless on older core — PHP ignores it.
The suite was not discovered
MissingGroupException, because 12 reads theGroupattribute rather than@group. Nothing ran atall. Five classes gained it, and the functional ones
RunTestsInSeparateProcesses.TagViewTestneededDataProviderattributesalongside its annotations rather than instead of them: PHPUnit 9 on
Drupal 10 reads only the annotation, PHPUnit 12 only the attribute. Replacing
them passes on 12 and breaks 10 — I did that first and only caught it by
running 10.
Results
Before this the suite did not run on 12 at all.
Two other changes worth calling out
phpcs and phpstan are
allow_failure: falsehere, so findings thatpredate this branch were turning the pipeline red. They are fixed rather than
left: three "String concat is not required" in the test files, and
TagBlock's deriver, which injected thedfp_tagstorage directly and held it as a property. It takes the entity type manager
now.
OPT_IN_TEST_NEXT_MAJORwas0, so there was no nextmajor job at all and CI would not have built against Drupal 12 even after this
lands. It is
1now, with_AUTORUN_NEXT_MINORand_AUTORUN_NEXT_MAJORset — opting in only declares the jobs, theautorun variables are what run them, and the template defaults those to
none. That is whycomposer (next minor)has beensitting at manual with everything downstream skipped.
Separately, and needing a maintainer rather than a patch: the repository's
default branch is
8.x-1.x, which is 26 commits behind3.0.xand not in the supported branches. Raised in#3619700.
Comment #5
marcelovaniComment #6
marcelovani