Problem/Motivation

#3497124: Deprecate expectDeprecation(), use PHPUnit's expectUserDeprecationMessage*() instead introduced DrupalTestCaseTrait, with the intent to provides methods common across all Drupal abstract base test classes.

Proposed resolution

Check for repeating code across base test classes that can be moved to the trait, and reduce boilerplate.

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3573954

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

mondrake created an issue. See original summary.

mondrake opened merge request !14798

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Active » Needs review

I was thinking to try and streamline how $this->root is populated across the different base classes, but not sure it's worth the effort. Feedback appreciated.

mondrake’s picture

mondrake
Location 🇮🇹
commented

For now, only moved setDebugDumpHandler as it was the same code repeated in three separate classes.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Seems straight forward

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Reviewed & tested by the community » Needs work

Thanks @smustgrave. I think we may move ::getDrupalRoot() into the new trait and deprecate TestRequirementsTrait - that trait is no longer doing what it says in the docs. Thoughts?

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs work » Needs review

Now the resolution of the $root property in tests to indicate the root path to Drupal has been hubbed into the new trait with a hooked property that we can use in D12 that has PHP 8.5 minimum.

ATM I added bypasses for PHPStan and PHPCS as they still have failures with hooked properties.

But the concept of the MR is reviewable.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Related issues: +#3575792: Bump PHPStan & friends to latest release

PHPStan failures are now gone after commit of #3575792: Bump PHPStan & friends to latest release.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Issue tags: +Major version only

Code uses property hooks, so can't target D11.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Sorry this one appears to need a rebase. If you ping me I can try and review faster. Review queue stays having 250 items in it lol .

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs work » Needs review

Rebased.

smustgrave’s picture

smustgrave commented

Thanks for the quick rebase. Could the CR get an update?

mondrake’s picture

mondrake
Location 🇮🇹
commented

CR updated.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

In that case I have no additional feedback.

Not sure if we want follow ups for what you and joachim were discussing in slack about.

dcam’s picture

dcam commented
Status: Reviewed & tested by the community » Needs work

Sorry guys. I was reviewing while that conversation was taking place. I found one issue and one minor suggestion/question.

dcam’s picture

dcam commented

For what it's worth, this is really interesting. It's the first use of property hooks that I've personally seen in the wild. Conceptually, I think this is great. I like that we're cutting out duplication.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs work » Needs review

Thanks!

Addressed @dcam points.

Not sure if we want follow ups for what you and joachim were discussing in slack about.

This is the link to the slack discussion: https://drupal.slack.com/archives/C079NQPQUEN/p1775051492082049
IMHO that's unrelated to this. If someone wants to move those classes, feel free to file issues.

dcam’s picture

dcam commented
Status: Needs review » Reviewed & tested by the community

Thank you for considering my feedback. This looks good to me.

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

quietone’s picture

quietone commented

I triaged this issue and found everything in order and no unanswered questions. I have updated credit.

I applied two suggestions to DrupalTestCaseTrait which was to move the "@todo" and the "phpcs:ignoreFile" lines to the top of the file. That was just to conform to the other instances of "phpcs:ignoreFile" is core.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Reviewed & tested by the community » Fixed

Always nice to see these little cleanups going in, making things incrementally better.

Committed and pushed fe682bee7a2 to main. 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.

  • longwave committed fe682bee on main 
    test: #3573954 Move common code across Drupal base test suite classes to...

longwave closed merge request !14798

quietone’s picture

quietone commented

Publish the CR

kingdutch’s picture

kingdutch
he/him
Primary language English
commented

This was tagged as major version only which makes other in-flight work that's trying to change how app-root works in a BC way for a minor version quite a bit more challenging, because there's no standardised way in tests across 11.x and 12.x that Drupal now uses to get the root and the new hardcoded root for DrupalTestCaseTrait::$root exists only in 12.x and not in 11.x, which uses TestRequirementsTrait and various other methods.

Paragraph removed since the trait isn't actually new. Additionally the new DrupalTestCaseTrait causes some confusion with TestSetupTrait which lives in Drupal\Core\Test and seems to be for public consumption since it's in the lib. However, the use-case of that trait seems to be more narrow. The similar naming may cause some confusion and the goals seem to have some overlap. I don't see any discussion around this in the issue and it may be good to still have that.

But my main question is whether this can be backported to Drupal 11, even though that may require not using property hooks. So that it does not block the work for #1792310: Wrong DRUPAL_ROOT with non-standard code structure and #2529170: [PP-1] Remove DrupalKernel::initializeRequestGlobals and replace base_root, base_url and base_path with a service.

Status: Fixed » Closed (fixed)

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