Mitigate Extension dependency on DRUPAL_ROOT

We used to put the root into the serialized extension itself, but this causes issues with moving files, see #2558247: Remove rebuildAll() and module install from UpdatePathTestBase. I guess calling to \Drupal::root() would fix the issue?

This patch checks for the Drupal root using \Drupal::root() if available or DRUPAL_ROOT if not. Wasn't sure if I need the check or could have just used \Drupal::root() so let me know if unnecessary. I haven't created a test as I've never done one before. Happy to give it a shot if someone points me in the right direction.

Thanks! New patch checks for the service first, and if there, uses it, and if not uses the constant.

Thanks for your help so far @Mile23. Never written a Kernel test (in fact, any test) so gonna do some reading now and will come back when I have questions :-)

So these are the steps that I think are required: Create an extension object, serialize it, then use the unserialize method on it to test that the service container app.root is used instead of the DRUPAL_ROOT constant.

I’ve been reading up on PhpUnit and have a broad understanding of setUp() and mocks and am wondering if I should (and if it’s possible) create a mock of an extension object, and a mock of a container and then set up a fake app.root for the container so that the call to \Drupal::root() gets the fake root. And the test then checks that this is used instead of the constant.

I've seen the following approach to mocking a service and creating a dummy container:

// Mock the ConfigFactory service.
 $this->configFactory = $this->getMockBuilder('\Drupal\Core\Config
\ConfigFactory')
 ->disableOriginalConstructor()
 ->getMock();
 $this->configFactory->expects($this->any())
 ->method('get')
 ->with('key.default_config')
 ->willReturn($this->config);
 // Create a dummy container.
 $this->container = new ContainerBuilder();
 $this->container->set('config.factory', $this->configFactory);
 \Drupal::setContainer($this->container);

And am wondering if I should do something similar. So some questions:

1) Is this the right way to set up the container?
2) How should I set up the extension object (or mock object) that we’re going to use the unserialize method on.
3) Do you know of any existing tests that do something similar, which I could look at?

Thanks :-)

Thanks for you help @Mile23 - you are awesome :-)

So here's the patch with the test. I ran locally and it passes. Some things I wasn't' sure of but thought I'd just get something up here so you could check the actual patch, questions below:

1) Is there a more appropriate way to create the extension object?
2) Is the way I serialize and unserialize correct?
3) I couldn't access the root property of the extension object as it's protected, so I created a ReflectionProperty. Let me know if there's a better way.
4) I wasn't sure whether the dummy app root should be created in setUp() or just in the test itself. setUp() seemed more appropriate because that's where we're setting up the test ;-) but it could have easily been created as a variable in the test function like so:

public function testServiceAppRouteUsage() {
 // Set a dummy container app.root.
 $dummy_app_root = '/dummy/app/root';
 $this->container->set('app.root', $dummy_app_root);
 $data = $this->extension->serialize();
 $this->extension->unserialize($data);
 $reflection_root = new \ReflectionProperty($this->extension, 'root');
 $reflection_root->setAccessible(true);
 self::assertEquals($dummy_app_root, $reflection_root->getValue($this->extension));
}

5) Do I need a @covers e.g. @covers ::unserialize or something like that in the test docblock comment?

Thanks!

1. +++ b/core/tests/Drupal/KernelTests/Core/Extension/ExtensionUnserializedServiceAppRootUsageTest.php
   @@ -0,0 +1,54 @@
   + */
   +class ExtensionUnserializedServiceAppRootUsageTest extends KernelTestBase {
   

   Can you extend an existing Extension test? Maybe ModuleHandlerTest?

2. +++ b/core/tests/Drupal/KernelTests/Core/Extension/ExtensionUnserializedServiceAppRootUsageTest.php
   @@ -0,0 +1,54 @@
   +  protected function setUp() {
   +    parent::setUp();
   

   If you extend ModuleHandlerTest, then it makes sense to do all this setup in the test itself.

3. +++ b/core/tests/Drupal/KernelTests/Core/Extension/ExtensionUnserializedServiceAppRootUsageTest.php
   @@ -0,0 +1,54 @@
   +    $reflection_root = new \ReflectionProperty($this->extension, 'root');
   +    $reflection_root->setAccessible(true);
   

   phpunit's readAttribute does the same thing. Search for other examples in core or look at CckFileTest.

4. @covers is always good.

Setting to NW for the readAttribute changes. The rest of my comments are more nits than anything.

New patch which uses phpunit's readAttribute instead of the ReflectionProperty, at @heddn's suggestion. Left the test as a Kernel test after discussion in irc.

Looks fine to me.

+++ b/core/tests/Drupal/KernelTests/Core/Extension/ExtensionUnserializedServiceAppRootUsageTest.php
@@ -0,0 +1,55 @@
+    self::assertEquals($this->dummyAppRoot, $this->readAttribute($this->extension, 'root'));

this should use $this->assertEquals, not self::

PHP support for calling instance methods statically is to be phased out in a future version, lets avoid adding it

For some background https://thephp.cc/news/2017/07/dont-call-instance-methods-statically

Thanks @larowlan. Changed to use $this->assertEquals instead of self::assertEquals.

1. I'm not sure why this is not a unit test given the aim is to prove the Extension can be used in unit tests where DRUPAL_ROOT is not defined. The container is mockable in unit tests.

2. +++ b/core/tests/Drupal/KernelTests/Core/Extension/ExtensionUnserializedServiceAppRootUsageTest.php
   @@ -0,0 +1,55 @@
   +  protected $dummyAppRoot;
   

   This is a constant - '/dummy/app/root' - it's best just to hard code this. I makes for less indirection in tests and therefore quicker understanding of what is actually being tested.

3. +++ b/core/tests/Drupal/KernelTests/Core/Extension/ExtensionUnserializedServiceAppRootUsageTest.php
   @@ -0,0 +1,55 @@
   +    // Instantiate an Extension object for testing unserialization.
   +    $this->extension = new Extension($this->container->get('app.root'), 'module', 'core/modules/system/system.info.yml', 'system.module');
   +    // Set a dummy container app.root for testing.
   +    $this->dummyAppRoot = '/dummy/app/root';
   +    $this->container->set('app.root', $this->dummyAppRoot);
   

   All of this could be in the actual test method - no? And then there wouldn't be a need for all the class properties.

Thanks @alexpott. I have recreated the patch so that it's a unit test and simplified as suggested. I moved everything into the test and removed the setup(). I hope this is in order?

Added a patch with the changes as mentioned by #25.

@Vidushi Mehta i have tested the path. The patch has passed the Php unit tests. Adding unit test screenshots.

Thanks Vidushi. I believe that @coversDefaultClass needs to be set on the class, so I have updated and created a new patch. Otherwise, I think your changes address all the issues picked up by @Mile23. I've also created an interdiff so you can see what I changed.

@foxtrotcharlie Thanks for making my mistake correct :)

How about going a bit further than this an scheduling the root property for removal and asking things to provide it to the ->load() method. It doesn't really belong here. Serializing DRUAPL_ROOT makes this information non-portable so if it's put in a cache there's a problem. So in Drupal 8 we add an optional argument to ->load() and use it if provided. But let's do this in a separate issue as at least this allows us to make forward steps.

Changing the title since we're not removing the dependency we really only mitigating it and to do that we're using the service locator anti-pattern.

Adding issue credit for reviews too.

Committed b097188 and pushed to 8.7.x. Thanks!

diff --git a/core/tests/Drupal/Tests/Core/Extension/ExtensionUnserializedServiceAppRootUsageTest.php b/core/tests/Drupal/Tests/Core/Extension/ExtensionUnserializedServiceAppRootUsageTest.php
index bbcd226273..e6b8815461 100755
--- a/core/tests/Drupal/Tests/Core/Extension/ExtensionUnserializedServiceAppRootUsageTest.php
+++ b/core/tests/Drupal/Tests/Core/Extension/ExtensionUnserializedServiceAppRootUsageTest.php
@@ -14,16 +14,16 @@
  */
 class ExtensionUnserializedServiceAppRootUsageTest extends UnitTestCase {
 
- /**
-  * Tests that the Extension class unserialize method uses the preferred root.
-  *
-  * When the Extension unserialize method is called on serialized Extension
-  * object data, test that the Extension object's root property is set to
-  * the container's app.root and not the DRUPAL_ROOT constant if the service
-  * container app.root is available.
-  *
-  * @covers ::unserialize
-  */
+  /**
+   * Tests that the Extension class unserialize method uses the preferred root.
+   *
+   * When the Extension unserialize method is called on serialized Extension
+   * object data, test that the Extension object's root property is set to the
+   * container's app.root and not the DRUPAL_ROOT constant if the service
+   * container app.root is available.
+   *
+   * @covers ::unserialize
+   */
   public function testServiceAppRouteUsage() {
     // The assumption of our test is that DRUPAL_ROOT is not defined.
     $this->assertFalse(defined('DRUPAL_ROOT'), 'Constant DRUPAL_ROOT is defined.');

Fixed indentation and comment flow on commit.

diff --git a/core/tests/Drupal/Tests/Core/Extension/ExtensionUnserializedServiceAppRootUsageTest.php b/core/tests/Drupal/Tests/Core/Extension/ExtensionUnserializedServiceAppRootUsageTest.php
old mode 100755
new mode 100644

Fixed file mode on commit.