Add a trait for autowiring properties in tests

Left a comment on the MR.

Like the idea; IMHO we should always explicitly add the #[Autowire()] attribute to the property.

If we do this, then I think we should consider adding the trait to KernelTestBase and BrowserTestBase.

Thanks for review, responding to both. Have also split up the setUp method in the trait so classes that have their own setUp method can just call autoSetUp without having parent::setUp called twice.

Added another MR based on suggestions from @mondrake in #5

I tried to use Symfony's #[Autowire] attribute to make things more explicit - PoC so did not change everything.

Ideally we could mark autowireProperties() with #[Before] and let PHPUnit call it prior to setUp, but the container is not yet defined at that point.

@mondrake I accidentally force pushed the wrong branch, can you push your branch back?

@mstrelan done!

mondrake → changed the visibility of the branch 3464357-autosetup to hidden.

Drupal\Tests\content_moderation\Kernel\WorkspacesContentModerationStateTest::testNonLangcodeEntityTypeModeration fails because it inherits from Drupal\Tests\content_moderation\Kernel\ContentModerationStateTest, but the inherited properties are not scanned for autowiring - we need to take care of inheritance especially for base test classes.

#14 is wrong. The inherited properties inherit the attributes too, https://3v4l.org/WYsXd. The problem was in mixing \Drupal::xxx calls with injected services and usage of traits. Maybe to be addressed but not here.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Rebased

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

rebased

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

rebased

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Sorry if this is wrong review

But see we are adding 3 new dependencies or 1? Not sure what the PinnedDevDependencies are for or why it has 3 changes but other composer changes have 1. But summary doesn't mention that. Not sure if it needs a dependency evaluation being it's symfony

Updated IS. There is one new direct dependency, on symfony/expression-language. This indirectly requires symfony/cache itself, which requires symfony/cache-contracts itself.

So tried to give a dependency evaluation based on the question I had in #core-development https://drupal.slack.com/archives/C079NQPQUEN/p1736797368660389

So looking at https://github.com/symfony/expression-language

I see they did releases pretty regularly (1-2 a month) but last one was v7.2.0 on Nov 29th, 2024. May be nothing just worth noting

I then went to symfony issue queue and searched for 'expression-language' and only found 7 https://github.com/symfony/symfony/issues?q=is%3Aissue%20state%3Aopen%20...

So believe this could be a safe addition.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

merged with 11.x, no conflicts

This shouldn't be used in functional tests, see 🌱 Use \Drupal consistently in tests Needs work . Didn't review the rest of the MR properly yet, that was the first thing I checked.

#33 is the reasoning that \Drupal::service should be used instead?

If so, would changing AutowirePropertyTrait from using $this->container to using \Drupal::xx be a correct solution?

🌱 Use \Drupal consistently in tests Needs work does not differentiate between kernel and functional, so is #33 valid for kernel ones too?

@mondrake the problem is that if you have a property at all, whether for the container itself or an individual service, it can get out of sync with the container in the site-under-test because that site is in a different php process.

If you use \Drupal:: directly in the test (after e.g. installing a module or something else that rebuilds the container), then it stays in sync because it's newly requested.

Ideally we would not be calling Drupal APIs from within phpunit in functional tests once we start making requests to the Drupal site (fine beforehand in setup etc.), but that ideal is very, very far from the current reality.

the problem is that if you have a property at all, whether for the container itself or an individual service, it can get out of sync with the container in the site-under-test because that site is in a different php process

that sounds like this should be won't fix, then.

The assumption of this issue is that properties referring to services in the container are ok to have, and so they can be autowired. But if there's a problem with that, this does not hold (and btw it also means there is a massive debt of usage to be cleaned up).