Problem/Motivation

ContainerBuilderTest is coupled to Symfony's tests, which means if they change their test, our test breaks. It also means we can't untangle the vendor/-in-core/ problem here: #2380389: Use a single vendor directory in the root

Currently it requires a file in vendor/, like this:

require_once __DIR__ . '../../../../../../vendor/symfony/dependency-injection/Symfony/Component/DependencyInjection/Tests/Fixtures/includes/classes.php';

This is very brittle.

Proposed resolution

Re-implement the test classes (BarClass, BazClass) for the tests, using a \Drupal namespace.

Remaining tasks

User interface changes

API changes

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because our tests depend on something 2.7 no longer has!
Issue priority Normal, even if its breaks at the moment.
Unfrozen changes Unfrozen because it only changes automated tests.
Disruption No disruption.
CommentFileSizeAuthor
#15 2445497_15.patch6.1 KBmile23
#15 interdiff.txt2.78 KBmile23
#7 interdiff.txt3.89 KBmile23
#7 2445497_6.patch5.66 KBmile23
#3 interdiff.txt5.68 KBmile23
#3 2445497_2.patch4.34 KBmile23
#1 2445497_1.patch2.2 KBmile23

Comments

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Status: Active » Needs review
StatusFileSize
new 2445497_1.patch2.2 KB

Adds a classes.php file right next to ContainerBuilderTest. ContainerBuilderTest then requires that file instead.

Status: Needs review » Needs work

The last submitted patch, 1: 2445497_1.patch, failed testing.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
StatusFileSize
new 2445497_2.patch4.34 KB
new interdiff.txt5.68 KB

SimpleTest seems to want me to change this around. So I will. Probably for the better anyway.

Adding two classes under the Fixture/ subnamespace.

ContainerBuilderTest also uses another BarClass class from Symfony, but refers to it through use rather than require_once, so that's livable.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Status: Needs work » Needs review
mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Issue summary: View changes
Issue tags: +Novice

Marking as novice, because it's easy to review.

dawehner’s picture

dawehner
Primary language German
commented 
+++ b/core/tests/Drupal/Tests/Core/DependencyInjection/ContainerBuilderTest.php
@@ -9,10 +9,10 @@
+use Symfony\Component\Routing\Tests\Fixtures\AnnotatedClasses\BarClass as AnnotatedBarClass;

Maybe a naive question, but if we want to decouple it from the symfony test, don't we want to not use their class as well then?

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
StatusFileSize
new 2445497_6.patch5.66 KB
new interdiff.txt3.89 KB

Not a naive question, really. :-)

I removed the AnnotatedBarClass entirely, because it wasn't really necessary.

Some coding standards improvements, too.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented

This patch still applies. Giving it another testbot go-round.

Mile23 queued 7: 2445497_6.patch for re-testing.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Issue summary: View changes
Issue tags: +Composer
dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community

Seems good for me

rteijeiro’s picture

rteijeiro commented 
+++ b/core/tests/Drupal/Tests/Core/DependencyInjection/Fixture/BarClass.php
@@ -0,0 +1,26 @@
+ * A stub class for use with ContainerBuilderTest.

Should this be:

Contains \Drupal\Tests\Core\DependencyInjection\Fixture\BarClass. ? 

  1. +++ b/core/tests/Drupal/Tests/Core/DependencyInjection/Fixture/BazClass.php
@@ -0,0 +1,36 @@
+ * A stub class for use with ContainerBuilderTest.

    And this: Contains \Drupal\Tests\Core\DependencyInjection\Fixture\BazClass. ? 

  1. +++ b/core/tests/Drupal/Tests/Core/DependencyInjection/Fixture/BarClass.php
@@ -0,0 +1,26 @@
+  protected $baz;

    Maybe this property needs documentation? 

  2. +++ b/core/tests/Drupal/Tests/Core/DependencyInjection/Fixture/BarClass.php
@@ -0,0 +1,26 @@
+  public function setBaz(BazClass $baz) {

    Missing docblock here? 

  3. +++ b/core/tests/Drupal/Tests/Core/DependencyInjection/Fixture/BarClass.php
@@ -0,0 +1,26 @@
+  public function getBaz() {

    Same here? 

  4. +++ b/core/tests/Drupal/Tests/Core/DependencyInjection/Fixture/BazClass.php
@@ -0,0 +1,36 @@
+  protected $foo;

    The same for this property, should we document it? 

  5. +++ b/core/tests/Drupal/Tests/Core/DependencyInjection/Fixture/BazClass.php
@@ -0,0 +1,36 @@
+  public function setFoo(Foo $foo) {

    And document this functions?

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Status: Reviewed & tested by the community » Needs work
joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented 
+++ b/core/tests/Drupal/Tests/Core/DependencyInjection/ContainerBuilderTest.php
@@ -9,10 +9,9 @@
+use Drupal\Tests\Core\DependencyInjection\Fixture\BazClass;
+use Drupal\Tests\Core\DependencyInjection\Fixture\BarClass;

@@ -22,40 +21,38 @@ class ContainerBuilderTest extends UnitTestCase {
-    $container->register('baz', 'BazClass')
+    $container->register('baz', '\Drupal\Tests\Core\DependencyInjection\Fixture\BazClass')
...
-    $container->register('bar', 'BarClass')
+    $container->register('bar', '\Drupal\Tests\Core\DependencyInjection\Fixture\BarClass')

Do you need the namespaced class in the register statements when you've added the use statements?

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt2.78 KB
new 2445497_15.patch6.1 KB

Do you need the namespaced class in the register statements when you've added the use statements?

Yes. The container doesn't know what namespaces you've used. It needs fully-quaified names.

Fixed the coding standards issues from #12.

It turns out nothing uses BazClass' accessors or statics, so I killed them.

Thanks!

dawehner’s picture

dawehner
Primary language German
commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

Let's update the issue summary. This issue is needed in order to be able to update to symfony 2.7

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Issue summary: View changes
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed 673f5b7 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

  • alexpott committed 673f5b7 on 8.0.x 
    Issue #2445497 by Mile23, dawehner: Decouple ContainerBuilderTest from...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.