Add DependencySerializationTrait

1. +++ b/core/tests/Drupal/Tests/Core/DependencyInjection/DependencySerializationTraitUnitTest.php
   @@ -2,39 +2,39 @@
    /**
     * @file
   - * Contains \Drupal\Tests\Core\DependencyInjection\DependencySerializationTest.
   + * Contains
   + * \Drupal\Tests\Core\DependencyInjection\DependencySerializationTraitUnitTest.
     */
   

   I really think we shouldn't move this on two lines. 3 characters more and it would be too long anyway ;)

2. +++ b/core/tests/Drupal/Tests/Core/DependencyInjection/DependencySerializationTraitUnitTest.php
   @@ -2,39 +2,39 @@
   -   * Tests serialization and unserialization.
   +   * @covers ::__sleep
   +   * @covers ::__wakeup
       */
   -  public function testSerialization() {
   +  public function test__WakeUp() {
   

   not sure why the method needs to be renamed? We can't name it after both methods and __sleep() is just as important ;)

3. +++ b/core/tests/Drupal/Tests/Core/DependencyInjection/DependencySerializationTraitUnitTest.php
   @@ -43,22 +43,12 @@ public function testSerialization() {
   -    // The original object got _serviceIds added so removing it to check
   -    // equality.
   -    unset($dependencySerialization->_serviceIds);
   -
   -    // Ensure dependency injected object remains the same after serialization.
   -    $this->assertEquals($dependencySerialization, $object);
   -
   -    // Ensure that _serviceIds does not exist on the object anymore.
   -    $this->assertFalse(isset($object->_serviceIds));
   -
   

   The reason it is declared on the class is that it can be public, so that this assertion can work.

   I don't think it should be removed. Maybe changed to assert with an empty array. or do = NULL instead of array() ?

There's an issue
#2206175: Document traits that use methods on interfaces
which is proposing a standard for how to document Traits.

The suggestions there (not official standards yet but probably a good idea):

a) Add a @see in the main Trait doc block referencing the interface whose methods it is implementing.

b) Document each method as:

/**
* Implements \Full\Namespaced\Interface\Name::methodName().
*/
function methodName() { }

Yeah, we updated the standards for cases like this where it's not implementing an interface. Discussion is ongoing...

Meanwhile, a minor issue to fix in the current patch, and a question:

a)DependencySerialization.php

+ * @deprecated As of Drupal 8.0, use
+ * \Drupal\Core\DependencyInjection\DependencySerializationTrait instead.
  */
 abstract class DependencySerialization {

After @deprecated the next line should be indented two spaces. (like after any other tag)

b) So you are replacing two different abstract classes with the same trait? How come we had two?

I don't see a second class, the other one is a test that's changed so much that it doesn't show up as renamed anymore apparently.

However, I would expect a second removal as mentioned in IRC, and that's in \Drupal\Core\Plugin\PluginBase.

My mistake on the second class. Not sure what I was looking at. Maybe I should put my glasses on. :)

Coming from #2210873: Let plugin bags use the DependencySerializationTrait (was: EditorAdminTest fails on PHP 5.4 silently with unicorn editor)

1. AFAICS, this trait will have to be used pretty much by every service, controller, form, plugin, and generally all classes that get services injected, because no class can know whether it is going to be serialized somewhere else...?

2. This trait introduces a dependency on the global state construct of \Drupal::getContainer() to every class that uses it...?

3. Usage of this trait means that a class cannot implement __sleep() + __wakeup() on its own anymore, unless it implements the following fugliness:

   class MyClass {
     use DependencyInjectionTrait {
       __sleep as __sleepDI;
       __wakeup as __wakeupDI;
     }
   
     public function __sleep() {
       // Perform necessary customizations to class members.
       ...
       return $this->__sleepDI();
     }
   
     public function __wakeup() {
       $this->__wakeupDI();
       // Restore class members.
       ...
     }
   
   }
   

   cf. http://php.net/manual/en/language.oop5.traits.php#language.oop5.traits.c...

Hrm. All of that sounds very concerning.

I wonder whether we shouldn't change this trait implementation to use the \Serializable interface instead?

→ Apply the trait to the injected services themselves, instead of each and every single consuming class?

trait DependencyInjectionTrait implements \Serializable {

  public function serialize() {
    if (isset($this->_serviceId) {
      return 'service:' . $this->_serviceId;
    }
    return serialize($this);
  }

  public function unserialize($data) {
    $data = unserialize($data);
    if (strpos($data, 'service:') === 0) {
      $this = \Drupal::service(substr($data, 8));
    }
  }

}

(untested pseudo code)

Or perhaps we have to rethink entirely — if the state of injected dependencies cannot be serialized/unserialized with other objects (because the state may differ in the context in which unserialization happens), then why are we storing those stateful objects on the serializable object in the first place?

That appears to be the root cause, and this entire futzing with serialization only exists to prevent serialization of class members that shouldn't be class members to begin with?

For example, why can't we do this instead?

class MyClass {

  public __construct(ModuleHandlerInterface $module_handler, ...) {
    $this->setModuleHandler($module_handler);
  }

  protected function moduleHandler(ModuleHandlerInterface $module_handler = NULL) {
    static $instance;
    if (isset($module_handler)) {
      $instance = $module_handler;
    }
    return $instance;
  }

  public function doSomething() {
    return $this->moduleHandler()->invokeAll('blah');
  }

}

→ No more stateful services in class members of the consuming class.

→ serialize() works as-is.

Only unserialize() needs to re-inject the services from the container.

I wonder whether we shouldn't change this trait implementation to use the
\Serializable interface instead?

\Serializable is messed up on 5.4+, or maybe we messed up but at least right now, we have to use __unserialize() in a lot of places instead of the interface.

sun: The root problem here is that we're serializing objects that we have no business serializing, ie, those that have big service chains. The base class this trait is replacing is already a disgusting hack. The real answer is to adjust our key serializable objects to not have direct dependencies on services.

@Crell: How do we do that? Forms are one of our key serializable objects, and very often have a dependency on a plugin manager and/or other services provided by the module. In fact isn't that precisely what a good UI/API separation within a module would always lead to?

Forms are one of our key serializable objects

Perhaps we can fix that: #2183275-17: Use cache for $form, kv for $form_state.

I totally forgot this issue was here and just redid it. :-/ However, I went ahead and converted all usages to the trait outright. Let's see if the bot likes it or not...

This patch JUST does a straight conversion; it doesn't change anything else about the base class/trait. Its existence is a code smell but that's a separate matter from just making it a trait.

14: drupal_2208115_14.patch queued for re-testing.

I'm having to copy this code into yet another class, :\

So we basically have to not have configFactory private on FormBase, which is not the end of the world by any means IMO.. OR, not have this issue. Using Serializable is not an option as we can't implement an interface from a trait. So then in that case we are back to what we have now anyway :)

Sorry, branch rebasing fail.

Another one crept in.

+++ b/core/lib/Drupal/Core/Plugin/PluginBase.php
@@ -57,7 +57,7 @@ public function __sleep() {
-   * @todo Remove when Drupal\Core\DependencyInjection\DependencySerialization
+   * @todo Remove when Drupal\Core\DependencyInjection\DependencySerializationTrait
    * is converted to a trait.

Should be pretty close now ;-)

hehe

+++ b/core/lib/Drupal/Core/Form/FormBase.php
@@ -38,14 +39,9 @@
   /**
    * The config factory.
    *
-   * This is marked private in order to force subclasses to use the
-   * self::config() method, which may be overridden to address specific needs
-   * when loading config. See \Drupal\Core\Form\ConfigFormBase::config() for an
-   * example of this.
-   *
    * @var \Drupal\Core\Config\ConfigFactoryInterface
    */
-  private $configFactory;
+  protected $configFactory;
 

Should we instead update the comment to state something like "Never use this directly, use self::config() instead"? It's still correct even if it's protected again.

Looks great otherwise, if there ever was a class that's destined to become a trait then it's this ;)

Note that I just RTBC'd #2283929: Clean up condition plugins to have schema support and to allow them to be optional, if that gets in first, we need to update that and add the trait there as well.

Thanks. Good point - added a comment to that effect back in.

Works for me, the linked issue above was committed, so we need to update the class there as well.

Nice, there we go.

Great, I think this is ready :)

Committed to 8.x. Thanks.

Now #2849674: Complex job in ViewExecutable::unserialize() causes data corruption going to remove the trait from ViewExecutable