Add a PHPunit test for not using Drupal\Core code in Drupal\Component

This shouldn't still be postponed I think.

Reroll of #2.

Those are just cosmetic fixes, and missing the entire test.

I agree this seems like a sound way to avoid regressions. Test looks good. Can somebody confirm though that all the cleanup here is still relevant/intended?

Great plan. Test passes when patch applied and fails when I deliberately place a file mentioning the core namespace in the component directory.

1. +++ b/core/tests/Drupal/Tests/Component/DrupalComponentTest.php
   @@ -0,0 +1,69 @@
   +      elseif ($file->getExtension() == 'php') {
   

   Use ===.

2. +++ b/core/tests/Drupal/Tests/Component/DrupalComponentTest.php
   @@ -0,0 +1,69 @@
   +      if ($line && !strpos($line, '@see ')) {
   +        $this->assertFalse(strpos($line, 'Drupal\\Core'));
   

   Use strpos(...) !== FALSE, also the assertion results in a rather unhelpful message. E.g. Failed asserting that 4 is false. Would it be possible to at least mention the path to the offending file? Maybe even the line number?

Great, thanks.

1. +++ b/core/tests/Drupal/Tests/Component/DrupalComponentTest.php
   @@ -0,0 +1,71 @@
   +    $line_counter = 0;
   +    foreach (new \SplFileObject($class_path) as $line) {
   +      ++$line_counter;
   

   I think it also would be possible to write foreach (... as $line_number => $line).

2. +++ b/core/tests/Drupal/Tests/Component/DrupalComponentTest.php
   @@ -0,0 +1,71 @@
   +        $this->assertFalse(strpos($line, 'Drupal\\Core'), "Found 'Drupal\\Core' class usage at line $line_counter in file $class_path");
   

   This is about the core namespace, not classes. Perhaps something like Illegal reference to 'Drupal\\Core' namespace in $class_path at line $line_number

   Also we need to use assertNotIdentical(FALSE, strpos(...), ...) for the same reasons pointed out in the previous review. Sorry that I did not realize that before.

Thank you steve!

"assertNotIdentical" not found using these test bases - are you implying that function needs to be created as it is in other places like views tests or can I link it in from somewhere?

I guess he was talking about assertNotSame

+++ b/core/tests/Drupal/Tests/Component/DrupalComponentTest.php
@@ -0,0 +1,69 @@
+/**
+ * General tests for Drupal\Component that can't go anywhere else.
+ *
+ * @group Drupal
+ * @group Component
+ */
+class DrupalComponentTest extends UnitTestCase {

... as I saw in another issue, we should expand this to do the same for the tests itself, so we ensure that also tests don't use anything from the core namespace.

+++ b/core/tests/Drupal/Tests/Component/DrupalComponentTest.php
@@ -0,0 +1,79 @@
+        $this->assertNotSame(TRUE, strpos($line, 'Drupal\\Core'), "Illegal reference to 'Drupal\\Core' namespace in $class_path at line $line_number");

Should be assertSame(FALSE, strpos(...), ...). Sorry for the confusion.

With this change, the test should detect the following violations in component tests:

$ git grep -n Core core/tests/Drupal/Tests/Component
core/tests/Drupal/Tests/Component/PhpStorage/PhpStorageTestBase.php:11:use Drupal\Core\PhpStorage\PhpStorageFactory;
core/tests/Drupal/Tests/Component/Plugin/PluginBaseTest.php:10:use Drupal\Core\PhpStorage\PhpStorageFactory;
core/tests/Drupal/Tests/Component/Uuid/UuidTest.php:15:use Drupal\Core\DependencyInjection\ContainerBuilder;

Funny, UuidTest does not seem to use the ContainerBuilder at all. I think this use statement is spurious and can be deleted. The same applies for PhpStorageFactory in PluginBaseTest.

Also it seems that PhpStorageTestBase::$storageFactory is never used, neither in the base class nor in any of the subclasses. That means that this property can be removed, along with the setUp() method and the use statement for PhpStorageFactory.

Is this meant for this issue?

As soon as the newly implemented unit test works correctly, it will detect the violations in the component test namespace and will fail because of that (which is good!). So either we need to open a new issue for fixing the offending tests (UuidTest, PluginBaseTest and PhpStorageTestBase) and get that one in before, or we can fix them in this issue. I propose to fix them here if possible and only open a new issue if this turns out to be too complex (i.e. requiring functional changes to the tests).

Great. From local testing I see there is one little thing left. Ironically, the new test detects a core reference in itself. The problem is the comment in line 64.

+++ b/core/tests/Drupal/Tests/Component/DrupalComponentTest.php
@@ -0,0 +1,79 @@
+   * Asserts that the file is not using any class from Drupal\Core.

Change it to something like Asserts that the file is not using any class from Core namespace and we should be fine.

Perfect. Thank you for picking that up and good luck with deyosemiting your box.

+++ b/core/tests/Drupal/Tests/Component/DrupalComponentTest.php
@@ -0,0 +1,79 @@
+  /**
+   * Tests that classes in Component Tests do not use any Core class.
+   */
+  public function testNoCoreInComponentTests() {
+    $component_path = dirname(substr(__DIR__, 0, -strlen(__NAMESPACE__))) . '/tests/Drupal/Tests/Component';
+    foreach ($this->findPhpClasses($component_path) as $class) {
+      $this->assertNoCoreUsage($class);
+    }
+  }
+

Thank you!

From local testing I see there is one little thing left. Ironically, the new test detects a core reference in itself.

Haha! :D

Great patch, thank you!

RTBC+1

A few nits that can be fixed on commit:

1. +++ b/core/lib/Drupal/Component/Plugin/Exception/InvalidDecoratedMethod.php
   @@ -1,7 +1,7 @@
   + * Definition of Drupal\Component\Plugin\Exception\InvalidDecoratedMethod.
   

   s/Definition of Drupal/Contains \Drupal\/

2. +++ b/core/tests/Drupal/Tests/Component/DrupalComponentTest.php
   @@ -0,0 +1,79 @@
   + * General tests for Drupal\Component that can't go anywhere else.
   

   s/Drupal\Component/\Drupal\Component/

3. +++ b/core/tests/Drupal/Tests/Component/DrupalComponentTest.php
   @@ -0,0 +1,79 @@
   + * @group Drupal
   

   We don't add @group Drupal anymore.

4. +++ b/core/tests/Drupal/Tests/Component/DrupalComponentTest.php
   @@ -0,0 +1,79 @@
   +   * Searches a directory recursively for php classes.
   

   s/php/PHP/

5. +++ b/core/tests/Drupal/Tests/Component/DrupalComponentTest.php
   @@ -0,0 +1,79 @@
   +   * Asserts that the file is not using any class from Core namespace.
   

   s/file/given class/

SortArray still needs quite a bit of cleaning up.

   * Callback for uasort() within:
   * - system_modules()
   * - theme_simpletest_test_table()

   * Callback for uasort() within system_admin_index().

   * Callback for uasort() used in various functions.

I think all the methods here should just have a line which says (if it is)

   * Callback for uasort().

Or something similar.

I guess someone might argue this is out-of-scope but since we doing:

+++ b/core/lib/Drupal/Component/Utility/SortArray.php
@@ -39,8 +39,6 @@ public static function sortByWeightElement(array $a, array $b) {
-   * Callback for uasort() within \Drupal\Core\Render\Element::children().
-   *

I think not.

+++ b/core/lib/Drupal/Component/Utility/SortArray.php
@@ -39,8 +39,6 @@ public static function sortByWeightElement(array $a, array $b) {
-   * Callback for uasort() within \Drupal\Core\Render\Element::children().
-   *

One final nitpick. Instead of removing this line entirely, it could be replaced with the comment now used elsewhere, i.e. Callback for uasort().

Thanks.

Committed 914b78d and pushed to 8.0.x. Thanks!

This issue is a unfrozen change (testing) as per #2350615: [policy, no patch] What changes can be accepted during the Drupal 8 beta phase? and it's benefits outweigh any disruption.