Split ForumBreadcrumbBuilder in two

linking directly to the contributor task for creating patch.

Oo. Awkward. Novice issue queue SHOWDOWN!

I made this before I saw that ThisIsDog had assigned himself. My bad.

But I made it so here goes.

ForumTest::verifyForums() leads me to believe that the tests cover breadcrumbs for chains with forums, containers, and nodes in them. I wish I could tell for sure, but that test class is impossible to read. Also: 2AM.

@ThisIsDog are you up to do a review on it?
Sometimes when that happens to me, I go ahead and finish, and then I diff my solution against someone else's. :) And that helps get the review started.

Review hints and explanations: https://drupal.org/contributor-tasks/review

Can we have unit tests as well? :$

+++ b/core/modules/forum/lib/Drupal/forum/ForumListingBreadcrumbBuilder.php
@@ -0,0 +1,91 @@
+  /**
+   * Configuration object for this builder.
+   *
+   * @var \Drupal\Core\Config\Config
+   */
+  protected $config;
+
+  /**
+   * Stores the Entity manager.
+   *
+   * @var \Drupal\Core\Entity\EntityManagerInterface
+   */
+  protected $entityManager;
+
+  /**
+   * The forum manager service.
+   *
+   * @var \Drupal\forum\ForumManagerInterface
+   */
+  protected $forumManager;
+
+  /**
+   * Constructs a new ForumBreadcrumbBuilder.
+   *
+   * @param \Drupal\Core\Entity\EntityManagerInterface $entity_manager
+   *   The entity manager.
+   * @param \Drupal\Core\Config\ConfigFactoryInterface $config_factory
+   *   The configuration factory.
+   * @param \Drupal\forum\ForumManagerInterface $forum_manager
+   *   The forum manager service.
+   */
+  public function __construct(EntityManagerInterface $entity_manager, ConfigFactoryInterface $config_factory, ForumManagerInterface $forum_manager) {
+    $this->entityManager = $entity_manager;
+    $this->config = $config_factory->get('forum.settings');
+    $this->forumManager = $forum_manager;
+  }

+++ b/core/modules/forum/lib/Drupal/forum/ForumNodeBreadcrumbBuilder.php
@@ -0,0 +1,88 @@
+  /**
+   * Configuration object for this builder.
+   *
+   * @var \Drupal\Core\Config\Config
+   */
+  protected $config;
+
+  /**
+   * Stores the Entity manager.
...
+   * @var \Drupal\Core\Entity\EntityManagerInterface
+   */
+  protected $entityManager;
+
+  /**
+   * The forum manager service.
+   *
+   * @var \Drupal\forum\ForumManagerInterface
+   */
+  protected $forumManager;
+
+  /**
+   * Constructs a new ForumBreadcrumbBuilder.
+   *
+   * @param \Drupal\Core\Entity\EntityManagerInterface $entity_manager
+   *   The entity manager.
+   * @param \Drupal\Core\Config\ConfigFactoryInterface $config_factory
+   *   The configuration factory.
+   * @param \Drupal\forum\ForumManagerInterface $forum_manager
+   *   The forum manager service.
+   */
+  public function __construct(EntityManagerInterface $entity_manager, ConfigFactoryInterface $config_factory, ForumManagerInterface $forum_manager) {
+    $this->entityManager = $entity_manager;
+    $this->config = $config_factory->get('forum.settings');
+    $this->forumManager = $forum_manager;
+  }

Similar much. Why not create a ForumBreadcrumbBuilderBase.

+++ b/core/modules/forum/lib/Drupal/forum/ForumListingBreadcrumbBuilder.php
@@ -0,0 +1,91 @@
+   * Constructs a new ForumBreadcrumbBuilder.

+++ b/core/modules/forum/lib/Drupal/forum/ForumNodeBreadcrumbBuilder.php
@@ -0,0 +1,88 @@
+   * Constructs a new ForumBreadcrumbBuilder.

Needs update. ForumListingBreadcrumbBuilder and ForumNodeBreadcrumbBuilder now.

Similar much. Why not create a ForumBreadcrumbBuilderBase.

See: http://drupalcode.org/project/drupal.git/blob/e93b685205fa5aa261953b5f14...

But I guess I'll refactor to have three classes where one was passing tests before. :-)

Sure, I'll do a review of what Mile23 submits. I actually fell asleep at my computer not too long after I assigned myself, so it actually works out okay. I don't want to step on his toes though, so I'll avoid writing code.

Feel free to ping me on IRC when you finish refactoring. I'll take a look at it.

A gift from your friendly PHPUnit fairy (you have to believe to receive).

Tests the node one, you might want to adapt this into an abstract base test and use it for both.

Also +1 to making a Forum breadcrumb base.

Forum breadcrumb base, and some more PHPUnit fairy goodness.

I struggled with testing build(), because of that despicable chain of dependencies.

From #8:

+++ b/core/modules/forum/tests/Drupal/forum/Tests/ForumNodeBreadcrumbBuilderTest.php
@@ -0,0 +1,233 @@
+  public function setUp() {
+    $this->forumNode = $this->getMockBuilder('Drupal\node\Entity\Node')
+      ->disableOriginalConstructor()
+      ->getMock();

This kind of stuff makes my test isolation OCD go nuts. :-) Maybe unwrap it to be a dataprovider and tack it on with the rest?

Also @ThisIsDog: I stepped on your toes, so please hop in where you want to.

Edit: Ugh. Wrong patch. Maybe after dinner.

interdiff please.

Interdiff when it's meaningful. :-)

I don't know what is the difference between #8 and #9 so can't review.

Please please please look after our reviewers and post an interdiff.

Moved the breadcrumb stuff to /Breadcrumb/ namespace (and folder) since there are a bunch of files.

Refactored larowlan's unit tests, and hammered out a few things as well.

Only needs a similar test for ForumListingBreadcrumbBuilder::build().

AND... Two interdiffs, one from #8 and one from #9, so choose based on which of those you read (or submitted).

Thank you very much @Mile23 for the interdiffs. Just some minor points.

1. +++ b/core/modules/forum/lib/Drupal/forum/Breadcrumb/ForumBreadcrumbBuilderBase.php
   @@ -0,0 +1,60 @@
   +   * Constructs a new forum breadcrumb builder.
   

   What's 'new' in it? :D

2. +++ b/core/modules/forum/tests/Drupal/forum/Tests/Breadcrumb/ForumNodeBreadcrumbBuilderTest.php
   @@ -0,0 +1,244 @@
   + * We don't test ForumNodeBreadcrumbBuilder::build() because we can't mock
   + * \taxonomy_term_load_parents_all().
   

   arghh. Can we redefine it in local namespace? comments please @larowlan.

3. +++ b/core/modules/forum/tests/Drupal/forum/Tests/Breadcrumb/ForumNodeBreadcrumbBuilderTest.php
   @@ -0,0 +1,244 @@
   +   * ¶
   

   white space.

4. +++ b/core/modules/forum/tests/Drupal/forum/Tests/ForumManagerTest.php
   @@ -12,6 +12,9 @@
   diff --git a/core/modules/node/lib/Drupal/node/Plugin/Search/NodeSearch.php b/core/modules/node/lib/Drupal/node/Plugin/Search/NodeSearch.php
   
   +++ b/core/modules/node/lib/Drupal/node/Plugin/Search/NodeSearch.php
   @@ -432,8 +432,7 @@ public function searchFormAlter(array &$form, array &$form_state) {
   diff --git a/core/modules/search/lib/Drupal/search/Tests/SearchLanguageTest.php b/core/modules/search/lib/Drupal/search/Tests/SearchLanguageTest.php
   
   +++ b/core/modules/search/lib/Drupal/search/Tests/SearchLanguageTest.php
   @@ -119,17 +60,6 @@ function testLanguages() {
   diff --git a/core/modules/search/lib/Drupal/search/Tests/SearchMultilingualEntityTest.php b/core/modules/search/lib/Drupal/search/Tests/SearchMultilingualEntityTest.php
   

   Wrong rebase I think.

We don't test ForumNodeBreadcrumbBuilder::build() because we can't mock \taxonomy_term_load_parents_all().

see #8

Stick it behind a protected method and then mock that method, passed in #8.

"We don't test ForumNodeBreadcrumbBuilder::build() because we can't mock \taxonomy_term_load_parents_all()."

see #8

Stick it behind a protected method and then mock that method, passed in #8.

Why would we do that, when forum manager does it for us? :-)

+++ b/core/modules/forum/lib/Drupal/forum/Breadcrumb/ForumNodeBreadcrumbBuilder.php
@@ -0,0 +1,51 @@
+    $parents = $this->forumManager->getParents($attributes['node']->forum_tid);

I think I missed deleting that comment you both are looking at.

And yes, bad rebase. Sob.

Ah, helps if I read the code and not the comments/reviews
Great work

Fixes #15 - note the interdiff is big because it backs out the search/node/field module changes.

Thanks, larowlan.

ForumBreadcrumbBuilderBase has a build() method that does the 'Home > Forum' part now, and all the build() methods are now tested.

Also maybe a real rebase. :-)

1. +++ b/core/modules/forum/lib/Drupal/forum/Breadcrumb/ForumListingBreadcrumbBuilder.php
   @@ -0,0 +1,49 @@
   +          $breadcrumb[] = $this->l($parent->label(), 'forum.page', array(
   +            'taxonomy_term' => $parent->id(),
   +              )
   

   It would be great to fix this weird indentation.

2. +++ b/core/modules/forum/tests/Drupal/forum/Tests/Breadcrumb/ForumBreadcrumbBuilderBaseTest.php
   @@ -0,0 +1,151 @@
   + * @see \Drupal\forum\ForumManager
   ...
   +   * Tests ForumBreadcrumbBuilderBase::__construct().
   

   We use @coversDefaultClass and @covers now in quite some places.

3. +++ b/core/modules/forum/tests/Drupal/forum/Tests/Breadcrumb/ForumBreadcrumbBuilderBaseTest.php
   @@ -0,0 +1,151 @@
   +
   +    // Reflect upon our properties.
   +    // We check that they're all not null, with some special attention to
   +    // config, which should return a test value.
   +    $reflector = new \ReflectionClass($builder);
   +    $property_names = array('entityManager', 'config', 'forumManager');
   +    $properties = array();
   +    foreach ($property_names as $property_name) {
   +      $ref_property = $reflector->getProperty($property_name);
   +      $ref_property->setAccessible(TRUE);
   +      $this->assertNotNull($ref_property->getValue($builder));
   +      // Special case for config.
   +      if ($property_name == 'config') {
   +        $config = $ref_property->getValue($builder);
   +        $this->assertEquals('IAmATestValue', $config->get('IAmATestKey'));
   +      }
   

   You can use assertAttributeEquals directly.

4. +++ b/core/modules/forum/tests/Drupal/forum/Tests/Breadcrumb/ForumListingBreadcrumbBuilderTest.php
   @@ -0,0 +1,235 @@
   +  public function providerTestApplies() {
   

   <3

All the stuff in #20 (including <3), plus there was more funky formatting.

summary of these changes: some typos, copy paste errors, standards. (so just nits)

details (to see all the changes, see interdiff):

1. +++ b/core/modules/forum/lib/Drupal/forum/Breadcrumb/ForumBreadcrumbBuilderBase.php
   @@ -0,0 +1,74 @@
   + * Forum breadcrum base class.
   
   +++ b/core/modules/forum/lib/Drupal/forum/Breadcrumb/ForumListingBreadcrumbBuilder.php
   @@ -0,0 +1,49 @@
   +/**
   + * Breadcrumb builder for forum listing pages.
   

   a) we might as well follow the standard for api summaries on classes. https://drupal.org/node/1354#classes "Use a third person verb to start the summary of a class, interface, or method. For example: "Represents a ..." or "Provides..."."

   b) a typo: breadcrum

2. +++ b/core/modules/forum/lib/Drupal/forum/Breadcrumb/ForumBreadcrumbBuilderBase.php
   @@ -0,0 +1,74 @@
   +   * Stores the Entity manager.
   +   *
   +   * @var \Drupal\Core\Entity\EntityManagerInterface
   +   */
   +  protected $entityManager;
   

   The entity manager.
   is more common, eh? Not sure why it was "Entity".

3. +++ b/core/modules/forum/lib/Drupal/forum/Breadcrumb/ForumListingBreadcrumbBuilder.php
   @@ -0,0 +1,49 @@
   +use Drupal\taxonomy\TermInterface;
   

   TermInterface is an unused use.

4. +++ b/core/modules/forum/lib/Drupal/forum/Breadcrumb/ForumListingBreadcrumbBuilder.php
   @@ -0,0 +1,49 @@
   +          $breadcrumb[] = $this->l($parent->label(), 'forum.page', array(
   +            'taxonomy_term' => $parent->id(),
   +            )
   +          );
   

   #20.1 didn't say how it should be fixed but #1539712: [policy, no patch] Coding standards for breaking function calls and language constructs across lines seems to imply that the closing parens should be at the same indention as the line that opened them.

5. +++ b/core/modules/forum/lib/Drupal/forum/Breadcrumb/ForumListingBreadcrumbBuilder.php
   @@ -0,0 +1,49 @@
   +        if ($parent->id() != $term_id) {
   +          $breadcrumb[] = $this->l($parent->label(), 'forum.page', array(
   +            'taxonomy_term' => $parent->id(),
   

   huh. I didn't know forums used taxonomy terms as their ids and for organization. interesting. @anypost says it was always like this.

6. +++ b/core/modules/forum/tests/Drupal/forum/Tests/Breadcrumb/ForumBreadcrumbBuilderBaseTest.php
   @@ -0,0 +1,162 @@
   +    // Test that the constructor made a config object
   +    // with our info in it.
   

   wrapping this to 80 chars.
   "Lines containing comments (including docblocks) must wrap as close to 80 characters as possible without going over,..."
   https://drupal.org/node/1354#general

7. +++ b/core/modules/forum/tests/Drupal/forum/Tests/Breadcrumb/ForumBreadcrumbBuilderBaseTest.php
   @@ -0,0 +1,162 @@
   +      ->will($this->returnValueMap(array(
   +          array('forums', $vocab_item),
   

   I think we just indent 2 spaces, even in a place like this.

8. +++ b/core/modules/forum/tests/Drupal/forum/Tests/Breadcrumb/ForumListingBreadcrumbBuilderTest.php
   @@ -0,0 +1,237 @@
   + * Contains \Drupal\forum\Tests\ForumManagerTest.
   

   copy and paste. fixed to match the class.

9. +++ b/core/modules/forum/tests/Drupal/forum/Tests/Breadcrumb/ForumNodeBreadcrumbBuilderTest.php
   @@ -0,0 +1,236 @@
   +    $breadcrumb_builder = $this->getMock('Drupal\forum\Breadcrumb\ForumNodeBreadcrumbBuilder', NULL, array($entity_manager, $config_factory, $forum_manager)
   +    );
   

   this

   );
   

   all by itself,
   is a little weird.

   we could split it like some of the others.
   or make it all one line.

   or maybe like...

       $builder = $this->getMockBuilder('Drupal\forum\Breadcrumb\ForumNodeBreadcrumbBuilder')
         ->setConstructorArgs(
           array(
             $entity_manager,
             $config_factory,
             $forum_manager,
           )
         )
         ->setMethods(NULL)
         ->getMock();
   

   (which is else where in this same class)

   I googled a bit to see if there was some advice about when to use getMock vs getMockBuilder,
   and found
   http://phpunit.de/manual/3.7/en/test-doubles.html
   and
   http://stackoverflow.com/questions/279493/phpunit-avoid-constructor-argu...
   In this case, I guess either could be used.

+++ b/core/modules/forum/tests/Drupal/forum/Tests/Breadcrumb/ForumBreadcrumbBuilderBaseTest.php
@@ -54,68 +56,77 @@ public function testConstructor() {
+
+    // Test that the constructor made a config object
+    // with our info in it.
+    $reflector = new \ReflectionClass($builder);
+    $ref_property = $reflector->getProperty('config');
+    $ref_property->setAccessible(TRUE);
+    $config = $ref_property->getValue($builder);
+    $this->assertEquals('IAmATestValue', $config->get('IAmATestKey'));
   }
 

You can also use assertAttributeEquals here.

@YesCT, #22.9: getMock vs getMockBuilder

Yah, the two bits do the same thing. They really should be consistent in the same patch, shouldn't they? I think the builder is more expressive and easier to read, but The Drupal Community's Opinion May Differ.

@dawehner #23: You can also use assertAttributeEquals here.

Actually you can't. The constructor takes a config factory and then extracts the config object and stores it. What we're testing is whether that happens.

Just amended ForumNodeBreadcrumbBuilderTest::testBuild() to have a nicer getMock() format.

This patch is doing some serious unit testing. Thanks for the awesome work @Mile23.

+++ b/core/modules/forum/lib/Drupal/forum/Breadcrumb/ForumNodeBreadcrumbBuilder.php
@@ -0,0 +1,47 @@
+ * Contains \Drupal\forum\Forum\Breadcrumb\NodeBreadcrumbBuilder.

ForumNodeBreadcrumbBuilder not NodeBreadcrumbBuilder.

@jibran: Doh! that one 'contains'....

Much unit testing credit goes to the PHPUnit fairy. Otherwise known as larowlan (see #8)

Thanks. I think it is done.

I went through it too and it looked good to me.

27: forum-bc-split-2209997_24.patch queued for re-testing.

Reverting the old status.

Reviewing...

Committed 58f31ac and pushed to 8.x. Thanks!