use ContainerAwareTrait instead of extending ContainerAware

@dawehner - Just before I jump on this and get ahead of myself.

Are you saying the template to follow should be :-

From core_servies.yml

  access_manager:
    class: Drupal\Core\Access\AccessManager
    arguments: ['@router.route_provider', '@url_generator', '@paramconverter_manager']
    calls:
      - [setContainer, ['@service_container']]
      - [setRequest, ['@?request']]

and this is an example of what needs to change ?

  keyvalue:
    class: Drupal\Core\KeyValueStore\KeyValueFactory
    arguments: ['@service_container', '@settings']

First Pass .. I hope I have them all...

With this patch drupal installs .. which shows a lot of functionality .. lets see what testbot has to say.

1) Removed all instances of "extends ContainerAware" in favour of "use ContainerAwareTrait"

2) For style and layout perspective using BrokenHandlerTrait as found in core as the template.. so removed fully qualified use of
"use \Symfony\Component\DependencyInjection\ContainerAwareTrait;"

3) most of the failing tests seem to come from DrupalUnitTest.php and is construction of KeyValueFactory, its constructor now need only a
single parameter and a set to inject the service_container.

      $container
        ->register('keyvalue', 'Drupal\Core\KeyValueStore\KeyValueFactory')
        ->addArgument(new Reference('settings'));
      $container->set('service_container', newReference('service_container'));

     $this->container
       ->register('keyvalue.expirable', 'Drupal\Core\KeyValueStore\KeyValueExpirableFactory')
-      ->addArgument(new Reference('service_container'))
+      ->addMethodCall('setContainer', new Reference('service_container'))
       ->addArgument(new Reference('settings'));
     // Define two data collections,

Now using AddMethodCall to inject the service_continaer correctly. Fixed in two other places.

Fixed malformed AddMethodCall

-      ->addMethodCall('setContainer', new Reference('service_container'))
+     ->addMethodCall('setContainer', array(new Reference('service_container')))

Fixed more tests.

Fixed a few more tests

CacheContextsTest and HttpKernelTest

class: Drupal\Core\KeyValueStore\KeyValueFactory
-    arguments: ['@service_container', '@settings']
+    arguments: ['@settings']
+    calls:
+      - [setContainer, ['@service_container']]

I don't think we should be making these classes use ContainerAware if they aren't already. Really, i don't understand why we're doing it at all. Why not just inject it in the constructor like KeyValueFactory does? If the object can't function at all without the container, it should use constructor injection.

From what I can tell, there isn't a scenario where setContainer is called again, so all this seems to be gaining us is a more cumbersome service definition.

Yeah I think replacing "extends ContainerAware" is about all we should do here. Any services using proper injection can be left alone.

Forgive me if I am 'Tilting at windmills"

I accept this is a minor, subtle point, but forcing me to make the case for using ContainerAwareTrait vs "extends ContainerAware" is no bad thing!

So I have updated the issue summary

In the next patch I will back out the changes unless in fits with only changing existing extended ContainerAware classes.

1) I have backed out CacheContext, KeyValueFactory and KeyValueFactoryExpirable. Lets see what the tesbot reports now.

2) I have also addressed a couple of cascaded bug each hiding each other in ThemeTestSubscriber - which incorrectly extended ContainerAware

ThemeTestSubscriber does not need a service_container it needs an IntrospectableContainer.

In any event theme_test.services.yml does not as expected inject a container of any form

The place where the uninitialised container is used is "unreached code" by the testbot hence no bug is picked up!

Since there is no need to dynamically inject a container into this class I have just added a todo in the constructor and removed the extends ContainerAware

If the testbot does not report a fail in this department I will file a separate bug report.

Resubmitting

Ok that blew up badly because of a failure to backout changes to KeyValueFactory that affected every unit test ( DrupalUnitTestBase)

Fixed

this should get back to the 70 fails of #15

This next patch is a test to prove a point ( it is not core quality code )

There is a another difference between "use ContainerAwareTrait" and "extends ContainerAware"

Only when using extends does it inherit "implements ContainerAwareInterface".

The bug I think I can see is that the controllerResolver picks up on this and at the appropriate time calls setContainer

This is the temporary change I have made to see if it resolves all the remaining issues.

-    if ($controller instanceof ContainerAwareInterface) {
+    if (method_exists($controller,'setContainer')) {
       $controller->setContainer($this->container);

Once again this change is just to highlight the issue not to solve it...

So I expect a division into two camps

To the "This issue is just needless fiddling" camp -- I think it strengthens your point.

To the "I like everything to be explicit when constructing my class" the fact that every class that may pass through ControllerResolver needs to be altered to explicitly add "implements ContainerAwareInterface" then this is a good thing...

Meh - I myself have no opinion...

Yes, we should definitely add "implements ContainerAwareInterface" to all of the classes using the trait.

Reverted #31, implemented #33 If this stays green, I will add what I expect to be the template for class definition, service build instructions and factory construction of containerAware classes to the issue summary.

and then leave it for review.

Patch looks great now, thanks!

I have a couple of points, that are very minor, but since isn't blocking anything we should fix them before commit I think.

Also you re-introduced the change of moving a lot of classes from constructor injection to setter injection for the container. I personally think it makes a lot of sense to consistently use setter injection in case, but there were some people that had reservations against that above. So maybe just leave that part out, so this doesn't get held up any longer?

1. +++ b/core/lib/Drupal/Core/Access/AccessManager.php
   @@ -26,7 +27,8 @@
   +class AccessManager implements ContainerAwareInterface {
   +  use ContainerAwareTrait;
   

   And elsewhere: There should be an empty line between these two.

2. +++ b/core/lib/Drupal/Core/HttpKernel.php
   @@ -29,31 +27,12 @@
   +class HttpKernel extends BaseHttpKernel implements ContainerAwareInterface
    {
   

   While we're at it, can we move the curly brace "{" on the same line as the "class" declaration, please?

Thank you gentleman for your comments...

So my task from 36 was to revert every case where I mangled the constructor and converted from constructor dependency injection to setter injection....
The changes in 37 affect every instance of that... so if I have analysed the situation correctly there is nothing more for me to do.

So onto the cosmetic changes...

regarding

class AccessManager implements ContainerAwareInterface {
   use ContainerAwareTrait;

versus

class AccessManager implements ContainerAwareInterface {
(new line)
   use ContainerAwareTrait;

I went looking for the definitive way forward here https://drupal.org/coding-standards
but could not find anything helpful.

I have copied the implicit standard, which is that in the very limited example of traits in core, currently,
the traits hugs the class definition...(see BrokenHandlerTait )

Can I humbly suggest that the uniform styling of traits is best done in a separate issue. Where more eyes will see it.

So that leaves #36.2 - ok no problem.

Minor Housekeeping

1) I have just been reading around the issue and just seen some of the policy discussions, and feels like I may have just blindly stepped on a landmine...I am linking here to prevent any new visitors from doing something similar

2) Linking to the bug that this issue discovered.

less hugging, more sanity

The class whitespace stuff is documented here: https://drupal.org/node/608152#indenting

...that means BrokenHandlerTrait is doing it wrong, not a biggie though.

Thanks for updating the patch nevertheless @martin107!

Since @dawehner agrees, being bold and marking RTBC. (Nice trick in the services.yml btw!)

Edit: crosspost

Reroll.

Reroll was trivial, nothing has changed -- so back to RTBC

+++ b/core/modules/system/tests/modules/theme_test/lib/Drupal/theme_test/EventSubscriber/ThemeTestSubscriber.php
@@ -24,6 +23,11 @@ class ThemeTestSubscriber extends ContainerAware implements EventSubscriberInter
+  public function __construct() {
+     // @todo - Must initialize $container.
+     // see onView()
+  }

This code looks unnecessary.

Removed __construct

Committed 12b53d0 and pushed to 8.x. Thanks!

Looks like https://drupal.org/node/1937056 could do with an update.

#52 change record updated

After this commit I am no longer able to access any Drupal-generated page without Apache crashing in a Windows 8 64-bit + Acquia Dev Desktop (Apache 2.2.22, PHP 5.4.8, MySQL 5.166-community) dev environment. The page is rendered and immediately after Apache dies (you can see the results attached).

I am pretty sure this is the issue causing this behavior: I identified it through git bisect and if I apply -R the committed patch to the latest HEAD everything works fine.

I've no idea of what might be going on, it seems we are triggering some weird PHP bug. Any suggestion?
(Possibly related issue: #2041375: Drupal 8 crashes apache 2.4.4 on 64bit).

No error in the error.log (obviously :), Windows syslog says:
(translating from italian)

Name of the application that generated the error: httpd.exe, version: 2.2.22.0, timestamp: 0x4f242d7a
Name of the module that generated the error: php5ts.dll, version: 5.4.8.0, timestamp: 0x507dd793
Exception code: 0xc0000005
Error offset: 0x00058e5f
ID of the process that generated the error: 0x13a8
Start time of the application that generated the error: 0x01cf5c30a8bdc760
Path of the application that generated the error: F:\bin\acquia-dd\apache\bin\httpd.exe
Path of the module that generated the error: F:\bin\acquia-dd\php5_4\php5ts.dll
Notice ID: ed1b1f2c-c823-11e3-bee7-f9dc4f72b177
Full name of the package that generated the error:
Application ID for the package that generated the error:

So, yes, I think this is the Windows equivalent of a segfault.

We're not alone... http://forumsarchive.laravel.io/viewtopic.php?id=11619

@plach please open an issue with your error and info with the tag
d8 dev environment
(http://drupal.org/project/issues/search?projects=&issue_tags=d8+dev+envi...)

the issue you open might be closed if this is reverted, or if another follow-up issue fixes it, but it is just the kind of d8 dev env thing that me and other people will run into when helping other people get setup with d8 locally. so having an issue will help.

There is also https://bugs.php.net/bug.php?id=60369 and https://bugs.php.net/bug.php?id=63002 - plach can you disable wncache or upgrade your php?

WinCache is not enabled but upgrading to PHP 5.4.27 does fix the issue :)

@YesCT:

Since it seems we have a fix here, what about just tagging this with "d8 dev environment"?

well, did this instead. :)

#2246775: Suggest 5.4.11 as minimum PHP version for Windows and MacOS if XCache is enabled

FWIW, this commit also broke installation for me on OSX 10.8, using PHP 5.4.10 with MAMP (2.1.2). I'm going to try upgrading MAMP and see if it resolves the issue.

Works for me

OSX 10.9.2
MAMP 2.2
PHP 5.5.3