Suggested commit message:

Issue #1951268 by larowlan, nick_schuch, jibran, dawehner, alexpott, tim.plunkett: Convert /forum and /forum/% to new routing system, remove forum_forum_load(), forum_get_topics(), create new Forum service.

Updated comment #101

Problem/Motivation

Brings forum module in to line with latest API improvements in core.

Proposed resolution

Move the two callbacks to a controller.
Add a ForumManager to store internal functions (previously prefixed with _).
Add public methods to the Manager for other forum api functions.
Put anything that performs queries inside the manager so backend can be swapped.
Remove forum_forum_load() and forum_get_topics() put logic into the Forum Manager service.
Convert /forum and /forum/% to new routing system.
Make $term->container a configurable (locked) boolean field and rename it to $term->forum_container to remove confusion regarding the DIC container.
#theme => 'forums' now expects #term instead of #tid.

Remaining tasks

Reviews of the new field

User interface changes

None

API changes

forum_forum_load() removed, just use entity_load('forum', $tid); instead.
To load the forum index (shown at /forum) which is a psuedo entity, use Drupal::service('forum_manager')->getIndex().
$forum->container is an entity field, so use $forum->container->value instead
_forum_node_check_node_type removed, use Drupal::service('forum_manager')->checkNodeType()
_forum_update_forum_index removed use Drupal::service('forum_manager')->updateIndex()
_forum_user_last_visit removed
_forum_topics_unread removed
forum_get_topics removed use Drupal::service('forum_manager')->getTopics()
_forum_get_topic_order removed
$term->container becomes $term->forum_container which is a boolean configurable (locked) field
theme('forums') expects a term instead of a tid

CommentFileSizeAuthor
#139 forum-page-route-1951268.139.interdiff.txt667 byteslarowlan
#139 forum-page-route-1951268.139.patch81.74 KBlarowlan
#137 forum-page-route-1951268.136.interdiff.txt1.04 KBlarowlan
#137 forum-page-route-1951268.136.patch81.27 KBlarowlan
#135 forum-page-route-1951268.134.interdiff.txt1.49 KBlarowlan
#135 forum-page-route-1951268.134.patch81.26 KBlarowlan
#133 forum-page-route-1951268.132.interdiff.txt792 byteslarowlan
#133 forum-page-route-1951268.132.patch81.25 KBlarowlan
#130 forum-page-route-1951268.reroll2.patch81.31 KBlarowlan
#125 forum-page-route-1951268.reroll.patch81.35 KBlarowlan
#121 forum-page-route-1951268.121.interdiff.txt3.98 KBlarowlan
#121 forum-page-route-1951268.121.patch84.46 KBlarowlan
#118 forum-page-route-1951268.118.interdiff.txt443 byteslarowlan
#118 forum-page-route-1951268.118.patch84.29 KBlarowlan
#115 forum-page-route-1951268.115.interdiff.txt1.04 KBlarowlan
#115 forum-page-route-1951268.115.patch84.71 KBlarowlan
#113 forum-page-route-1951268.113.interdiff.txt5.87 KBlarowlan
#113 forum-page-route-1951268.113.patch84.66 KBlarowlan
#110 forum-page-route-1951268.110.interdiff.txt638 byteslarowlan
#110 forum-page-route-1951268.110.patch85.26 KBlarowlan
#103 forum-page-route-1951268.103.interdiff.txt1.3 KBlarowlan
#103 forum-page-route-1951268.103.patch85.37 KBlarowlan
#100 forum-page-route-1951268.100.interdiff.txt27.16 KBlarowlan
#100 forum-page-route-1951268.100.patch84.47 KBlarowlan
#100 before-index.png23.82 KBlarowlan
#100 before-page.png23.23 KBlarowlan
#100 after-index.png23.94 KBlarowlan
#100 after-page.png21.01 KBlarowlan
#97 forum-page-route-1951268.97.patch71.03 KBlarowlan
#93 forum-page-route-1951268.93.interdiff.txt6.45 KBlarowlan
#93 forum-page-route-1951268.93.patch70.99 KBlarowlan
#79 forum-page-route-1951268.79.interdiff.txt2.64 KBlarowlan
#79 forum-page-route-1951268.79.patch68.01 KBlarowlan
#76 forum-page-route-1951268.76.interdiff.txt11.53 KBlarowlan
#76 forum-page-route-1951268.76.patch67.94 KBlarowlan
#69 forum-page-route-1951268.69.interdiff.txt10.08 KBlarowlan
#69 forum-page-route-1951268.69.patch62.1 KBlarowlan
#57 forum-page-route-1951268.57.interdiff.txt1.45 KBlarowlan
#57 forum-page-route-1951268.57.patch47.78 KBlarowlan
#52 forum-page-route-1951268.52.interdiff.txt662 byteslarowlan
#52 forum-page-route-1951268.52.patch47.76 KBlarowlan
#52 forum-page-route-1951268.interdiff.44-52.txt15.68 KBlarowlan
#50 forum-page-route-1951268.50.interdiff.txt1.36 KBlarowlan
#50 forum-page-route-1951268.50.patch47.75 KBlarowlan
#47 forum-page-route-1951268.47.interdiff.txt951 byteslarowlan
#47 forum-page-route-1951268.47.patch47.79 KBlarowlan
#45 forum-page-route-1951268.45.interdiff.txt2.45 KBlarowlan
#45 forum-page-route-1951268.interdiff.44-and-45.txt15.78 KBlarowlan
#45 forum-page-route-1951268.45.patch47.78 KBlarowlan
#44 forum-page-route-1951268.44.interdiff.txt13.02 KBnick_schuch
#44 forum-page-route-1951268.44.patch47.67 KBnick_schuch
#40 forum-page-route-1951268.40.interdiff.txt8.12 KBlarowlan
#40 forum-page-route-1951268.40.patch47.46 KBlarowlan
#39 forum-page-route-1951268.39.interdiff.txt5.06 KBlarowlan
#39 forum-page-route-1951268.39.patch46.41 KBlarowlan
#35 forum-page-route-1951268.33.interdiff.txt12.2 KBlarowlan
#35 forum-page-route-1951268.34.patch46.44 KBlarowlan
#32 forum-page-route-1951268.32.interdiff.txt445 byteslarowlan
#32 forum-page-route-1951268.32.patch38.81 KBlarowlan
#29 forum-page-route-1951268.29.interdiff.txt10.32 KBlarowlan
#29 forum-page-route-1951268.29.patch38.8 KBlarowlan
#22 forum-page-route-1951268.22.interdif.txt4.34 KBlarowlan
#22 forum-page-route-1951268.22.patch37.02 KBlarowlan
#20 forum-page-route-1951268.20.interdiff.txt794 byteslarowlan
#20 forum-page-route-1951268.20.patch38.17 KBlarowlan
#16 forum-page-route-1951268.16.interdiff.txt41.22 KBlarowlan
#16 forum-page-route-1951268.16.patch38.17 KBlarowlan
#1 forum-page-route-1951268.1.patch25.26 KBlarowlan
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

larowlan’s picture

The patch makes more sense than the issue summary.
Note that the 'forum' entity type can be loaded as both a term (using entity_load('taxonomy_term', $tid)) and a forum (entity_load('forum', $tid)) the difference being the 'forum' entity type adds the extra features (container, parents, forums) previously added to the term in forum_forum_load(), this moves them to first class entity properties and the logic into the storage controller (which can be swapped out in contrib).

larowlan’s picture

Issue summary: View changes

Updated issue summary.

andypost’s picture

Awesome! The only thing here that needs deeper review is usage of term-reference fields on forum nodes.

+++ b/core/modules/forum/lib/Drupal/forum/Controller/ForumController.phpundefined
@@ -0,0 +1,144 @@
+    drupal_set_title($forum->label());
...
+    drupal_set_title($vocabulary->label());

not sure it's good to set title from controller

+++ b/core/modules/forum/lib/Drupal/forum/Controller/ForumController.phpundefined
@@ -0,0 +1,144 @@
+    drupal_set_breadcrumb($breadcrumb);

same here

larowlan’s picture

Yes re field agreed, something to tackle in the entity form controller follow up.
Will put todo for title and breadcrumb for cleanup in render controller.

andypost’s picture

Also I think this will need a special access controller. And #1947432: Add a generic EntityAccessCheck to replace entity_page_access() should help to minimize a set of access callbacks

Crell’s picture

I *love* this concept. :-)

+++ b/core/modules/forum/lib/Drupal/forum/Controller/ForumController.php
@@ -0,0 +1,144 @@
+  /**
+   * Returns a forum page.
+   *
+   * @param \Drupal\forum\Plugin\Core\Entity\Forum $forum
+   *   (optional) The forum to render the page for. If empty, the forum index is
+   *   rendered instead. Defaults to NULL.
+   */
+  public function forumPage(Forum $forum = NULL) {

Needs a @return.

+++ b/core/modules/forum/lib/Drupal/forum/Controller/ForumController.php
@@ -0,0 +1,144 @@
+      $topics = forum_get_topics($forum->id(), $sort_by, $forum_per_page);

forum_get_topics() looks like it can be service-ified, or else made into a method of one of these classes.

+++ b/core/modules/forum/lib/Drupal/forum/ForumStorageController.php
@@ -0,0 +1,261 @@
+    // Query "Last Post" information for this forum.
+    $query = db_select('node', 'n');
+    $query->join('forum', 'f', 'n.vid = f.vid AND f.tid = :tid', array(':tid' => $tid));
+    $query->join('node_comment_statistics', 'ncs', 'n.nid = ncs.nid');
+    $query->join('users', 'u', 'ncs.last_comment_uid = u.uid');
+    $query->addExpression('CASE ncs.last_comment_uid WHEN 0 THEN ncs.last_comment_name ELSE u.name END', 'last_comment_name');

This should be using an injected connection and $this->database->select().

+++ b/core/modules/forum/lib/Drupal/forum/ForumStorageController.php
@@ -0,0 +1,261 @@
+      $query = db_select('node', 'n');
+      $query->join('node_comment_statistics', 'ncs', 'n.nid = ncs.nid');
+      $query->join('forum', 'f', 'n.vid = f.vid');

Ibid.

+++ b/core/modules/forum/lib/Drupal/forum/Plugin/Core/Entity/Forum.php
@@ -0,0 +1,84 @@
+   * Container flag.
+   *
+   * @var bool
+   */
+  public $container = FALSE;

I know this is an existing concept in Forum, but I initially confused it with the DIC. I don't think we can reasonably rename the concept throughout the forum system at this point, but could we rename the variable at least to $forumContainer or something like that? Everywhere else, $container means the DIC.

+++ b/core/modules/forum/lib/Drupal/forum/Plugin/Core/Entity/Forum.php
@@ -0,0 +1,84 @@
+  /**
+   * Overrides \Drupal\taxonomy\Plugin\Core\Entity\Term::uri().
+   */
+  public function uri() {
+    return array(
+      'path' => 'forum/' . $this->id(),
+    );
+  }

As it should be!

amateescu’s picture

+++ b/core/modules/forum/lib/Drupal/forum/Plugin/Core/Entity/Forum.php
@@ -0,0 +1,84 @@
+/**
+ * Defines the forum entity.
+ *
+ * @Plugin(
+ *   id = "forum",
+ *   label = @Translation("Forum"),
+ *   bundle_label = @Translation("Vocabulary"),
+ *   module = "forum",
+ *   controller_class = "Drupal\forum\ForumStorageController",
+ *   render_controller_class = "Drupal\taxonomy\TermRenderController",
+ *   access_controller_class = "Drupal\taxonomy\TermAccessController",
+ *   form_controller_class = {
+ *     "default" = "Drupal\forum\ForumFormController"
+ *   },
+ *   translation_controller_class = "Drupal\taxonomy\TermTranslationController",
+ *   base_table = "taxonomy_term_data",
+ *   uri_callback = "forum_uri",
+ *   fieldable = TRUE,
+ *   translatable = TRUE,
+ *   entity_keys = {
+ *     "id" = "tid",
+ *     "bundle" = "vid",
+ *     "label" = "name",
+ *     "uuid" = "uuid"
+ *   },
+ *   bundle_keys = {
+ *     "bundle" = "vid"
+ *   },
+ *   menu_base_path = "admin/structure/forum/edit/forum/%forum",
+ *   permission_granularity = "bundle"
+ * )
+ */
+class Forum extends Term {

Whoa.. two *different* entity types sharing the same base table? Shouldn't we be pushing for making entity subtypes (former bundles) classed objects instead? That way, we would still have the ability to do class Forum extends Term, but in a much cleaner (inheritance concerned) way.

Re #5:

This should be using an injected connection and $this->database->select().

This needs to happen for all entity storage controllers, and I don't think this is the issue that needs to deal with it.

larowlan’s picture

Status: Needs review » Postponed
ParisLiakos’s picture

Very nice..seems i should do something like that in #15266: Replace aggregator category system with taxonomy ?

yched’s picture

Wondering how views will cope with separate entity types using the same base table. AFAICT, it.might very well consider that all term entities are eligible on a views that lists forum entities (since they are in the "right" base table) ?

fubhy’s picture

Status: Postponed » Needs work

I agree with @amateescu on this one... Sharing a base table across multiple entity types is definitely not the right thing to do here and also comes with lots of problems also in regards to data integrity. Think about entity hooks for example. Deleting a Forum entity invokes the deletion hooks using 'forum' as entity type argument while it's actually a term in disguise... And there is more to it than just that.

Having said that I am actually 100% convinced that Forums should not leverage Terms in ANY way actually. I am not sure why they use Terms at all. Just because both need a hierarchical drag&drop UI that doesn't magically make Forums Terms. Forums should really be their own, decoupled entity types without any relation to Terms whatsoever (except if you want to tag your forum, obviously :P).

andypost’s picture

Suppose it's much easy to convert forum to own entity type as it was planned.
Forum already have separate admin-forms and forum_index table.
Taxonomy reference should be moved to ER so no matter where it would point.
The benefits could be good - forum entities could be specially optimized for performance

Berdir’s picture

Trying to summarize my thoughts from various discussions in IRC related to this.

1) The bundle filter problem in load is self-caused because we're trying to fake a different entity type where there actually isn't one.

2) Various discussions around per-bundle entity classes and controllers do not seem related to this issue because I don't see how they can solve problems that we can not already solve with the tools we have: (load) hooks and access checks.

3) Agree with others that forum storage is the Drupal 6 style of doing things (extending the core data structures like terms with separate tables) as opposed to providing specialized entity types. It should be changed but not in this issue I assume?. I'm not even sure if we can still do it in 8.x

I hope that makes sense so far :)

I also think if there are structural changes necessary to how forum terms work, then we should postpone in on the taxonomy term NG issue.

larowlan’s picture

Title: Convert /forum and /forum/% to new routing system, convert Forum to subclass Term entity type » Convert /forum and /forum/% to new routing system, remove forum_forum_load(), forum_get_topics(), create new Forum service

An update on how we're going to tackle this in Drupal 8:

  • forum_forum_load() is removed, logic is split between forum_taxonomy_term_load() for forums and a Forum service for the index.
  • Much of the storage controller specific logic in the above patch goes into a Forum service, forum_taxonomy_term_load() calls into the service for code-reuse purposes. Much of the original patch can be re-used then.
  • forum_get_topics() gets moved to the Forum service.
  • /forum/% and /forum moved to new ForumController and route

Yes the issue is tackling a lot in one patch, but the three are intimately linked and it doesn't make sense to do them individually.

Follow-ups become

A new entity type for Forum is definitely Drupal 9 material.

larowlan’s picture

Issue summary: View changes

Updated issue summary.

larowlan’s picture

Status: Needs work » Postponed

Updated issue summary, also this is definitely postponed pending #1818560: Convert taxonomy entities to the new Entity Field API

larowlan’s picture

Status: Postponed » Active

Back on this

larowlan’s picture

Status: Active » Needs review
FileSize
38.17 KB
41.22 KB

So this is a fairly major patch as follows:

  • forum_forum_load() is removed, logic is split between forum_taxonomy_term_load() for forums and a Forum Manager service.
  • Much of the storage controller specific logic in the above patch goes into the Forum Manager service, forum_taxonomy_term_load() calls into the service for code-reuse purposes.
  • forum_get_topics() gets moved to the Forum service.
  • _forum_get_topic_order() is moved to the service
  • _forum_user_last_visit() is moved to the service
  • /forum/% and /forum moved to new ForumController and route
  • There are still some calls to procedural functions in the service/route, I've tried to put these behind methods where possible

Status: Needs review » Needs work
Issue tags: -Needs architectural review, -Entity system, -Platform Initiative, -WSCCI-conversion

The last submitted patch, forum-page-route-1951268.16.patch, failed testing.

larowlan’s picture

Status: Needs work » Needs review

#16: forum-page-route-1951268.16.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Needs architectural review, +Entity system, +Platform Initiative, +WSCCI-conversion

The last submitted patch, forum-page-route-1951268.16.patch, failed testing.

larowlan’s picture

Status: Needs work » Needs review
FileSize
38.17 KB
794 bytes

One fail is the HEAD fail (since fixed)
The other was missing a \ on new \StdClass()

dawehner’s picture

+++ b/core/modules/forum/forum.moduleundefined
@@ -93,18 +93,11 @@ function forum_theme() {
-    'title callback' => 'entity_page_label',
-    'title arguments' => array(1),

Is this change intended? I am confused that there is title and title callback

+++ b/core/modules/forum/forum.moduleundefined
@@ -118,6 +111,7 @@ function forum_menu() {
+  // @todo route and form controllerify this.

@@ -127,6 +121,7 @@ function forum_menu() {
+  // @todo route and form controllerify this.

@@ -136,6 +131,7 @@ function forum_menu() {
+  // @todo route and form controllerify this.

@@ -143,6 +139,7 @@ function forum_menu() {
+  // @todo route and form controllerify this.

@@ -150,6 +147,7 @@ function forum_menu() {
+  // @todo route and form controllerify this.

Can we just skip this parts ... there is an issue for that

+++ b/core/modules/forum/forum.moduleundefined
@@ -168,48 +166,52 @@ function forum_menu_local_tasks(&$data, $router_item, $root_path) {
+    $vid = config('forum.settings')->get('vocabulary');

@@ -285,6 +287,29 @@ function forum_node_view(EntityInterface $node, EntityDisplay $display, $view_mo
+  $config = config('forum.settings');
+  $vid = $config->get('vocabulary');
...
+    if (in_array($forum_term->id(), $config->get('containers'))) {

It would be possible to directly use Drupal::config()

+++ b/core/modules/forum/forum.moduleundefined
@@ -168,48 +166,52 @@ function forum_menu_local_tasks(&$data, $router_item, $root_path) {
+    $field = field_info_field('taxonomy_forums');

What about calling "Field::fieldInfo()->getField()" directly?

+++ b/core/modules/forum/forum.routing.ymlundefined
@@ -10,3 +10,15 @@ forum_settings:
+forum_page:
+  pattern: 'forum'
+  defaults:
+    _content: 'Drupal\forum\Controller\ForumController::forumPage'
+  requirements:
+    _permission: 'access content'
+forum_forum_page:
+  pattern: 'forum/{taxonomy_term}'
+  defaults:
+    _content: 'Drupal\forum\Controller\ForumController::forumPage'
+  requirements:
+    _permission: 'access content'

Isn't it possible to just have a single route with ~ as default value?

+++ b/core/modules/forum/lib/Drupal/forum/ForumManager.phpundefined
@@ -0,0 +1,469 @@
+  function lastVisit($nid) {

public or protected?

larowlan’s picture

Is this change intended? I am confused that there is title and title callback

Yeah entity_page_label doesn't work with routes.
We added a drupal_set_title() instead.

Fixed rest of suggestions.

Status: Needs review » Needs work
Issue tags: -Needs architectural review, -Entity system, -Platform Initiative, -WSCCI-conversion

The last submitted patch, forum-page-route-1951268.22.patch, failed testing.

larowlan’s picture

Status: Needs work » Needs review
Issue tags: +Needs architectural review, +Entity system, +Platform Initiative, +WSCCI-conversion

#22: forum-page-route-1951268.22.patch queued for re-testing.

Crell’s picture

Status: Needs review » Needs work
+++ b/core/modules/forum/lib/Drupal/forum/Controller/ForumController.php
@@ -0,0 +1,166 @@
+    // Breadcrumb navigation.
+    $vocabularies = $this->entityManager->getStorageController('taxonomy_vocabulary')->load(array($this->config->get('vocabulary')));
+    $vocabulary = reset($vocabularies);
+    $breadcrumb[] = l(t('Home'), NULL);
+    $breadcrumb[] = l($vocabulary->label(), 'forum');
+    foreach (array_reverse($taxonomy_term->parents) as $parent) {
+      if ($parent->id() != $taxonomy_term->id()) {
+        $breadcrumb[] = l($parent->label(), 'forum/' . $parent->id());
+      }
+    }
+    drupal_set_breadcrumb($breadcrumb);

The breadcrumb rewrite is RTBC, so I think it's better to hold this until that's in and then rewrite this. It's already in the new breadcrumb patch.

In fact, that patch will likely conflict with this one. :-)

+++ b/core/modules/forum/lib/Drupal/forum/ForumManager.php
@@ -0,0 +1,469 @@
+   *
+   * @param int $sortby
+   *   One of the following integers indicating the sort criteria:
+   *   - 1: Date - newest first.
+   *   - 2: Date - oldest first.
+   *   - 3: Posts with the most comments first.
+   *   - 4: Posts with the least comments first.

These can totally be turned into constants on this class rather than just a docblock. Then the constants can be used in the code, and used by other code elsewhere, too!

jibran’s picture

jibran’s picture

Issue tags: +Needs reroll
+++ b/core/modules/forum/lib/Drupal/forum/Controller/ForumController.phpundefined
@@ -0,0 +1,166 @@
+  public function forumPage(Term $taxonomy_term = NULL) {
...
+  protected function forumForumPage(Term $taxonomy_term) {

This should be TermInterface

+++ b/core/modules/forum/lib/Drupal/forum/Controller/ForumController.phpundefined
@@ -0,0 +1,166 @@
+    $taxonomy_term->forums = $this->forumManager->getChildren($this->config->get('vocabulary'), $taxonomy_term->id());
+    $taxonomy_term->parents = $this->forumManager->getParents($taxonomy_term->id());
+    // Page title.
+    drupal_set_title($taxonomy_term->label());
+    // Breadcrumb navigation.
+    $vocabularies = $this->entityManager->getStorageController('taxonomy_vocabulary')->load(array($this->config->get('vocabulary')));
+    $vocabulary = reset($vocabularies);
+    $breadcrumb[] = l(t('Home'), NULL);
+    $breadcrumb[] = l($vocabulary->label(), 'forum');
+    foreach (array_reverse($taxonomy_term->parents) as $parent) {
+      if ($parent->id() != $taxonomy_term->id()) {
+        $breadcrumb[] = l($parent->label(), 'forum/' . $parent->id());
+      }
+    }

#1947536: Convert drupal_get_breadcrumb() and drupal_set_breadcrumb() to a service is committed now we can inject forum.breadcrumb

Crell’s picture

Issue tags: -Needs reroll

No need to inject forum.breadcrumb. That code just goes away entirely as it's now handled independently by the breadcrumb service.

larowlan’s picture

Status: Needs work » Needs review
FileSize
38.8 KB
10.32 KB

I'm getting a weird local failure to do with 'Add new Forum topic' but the given link is actually there. Hoping it is something weird local.
Fixes #25 and #27, reverts changes to routing.yml at #21 as that caused fails.
Also some other cleanup.

Status: Needs review » Needs work

The last submitted patch, forum-page-route-1951268.29.patch, failed testing.

andypost’s picture

How this affect usage of uri(), @larowlan please chime in #2010132: Canonical taxonomy term link for forum vocabulary is broken

larowlan’s picture

Status: Needs work » Needs review
FileSize
38.81 KB
445 bytes

Missed the route_name in hook_menu, nothing weird about it - was clicking the wrong 'Verbose output' link in Simpletest UI :), teach me to code while watching TV!

jibran’s picture

+++ b/core/modules/forum/lib/Drupal/forum/ForumBreadcrumbBuilder.phpundefined
@@ -47,31 +47,26 @@ public function __construct(EntityManager $entity_manager, ConfigFactory $config
+      if (_forum_node_check_node_type($node)) {

Can we move _forum_node_check_node_type to manger?

dawehner’s picture

Some additional points.

+++ b/core/modules/forum/lib/Drupal/forum/Controller/ForumController.phpundefined
@@ -0,0 +1,144 @@
+   * @var Drupal\forum\ForumManager

Missing starting "\"

+++ b/core/modules/forum/lib/Drupal/forum/Controller/ForumController.phpundefined
@@ -0,0 +1,144 @@
+  /**
+   * Vocabulary storage controller.
+   *
+   * @var \Drupal\taxonomy\VocabularyStorageControllerInterface

These lines are redundant.

+++ b/core/modules/forum/lib/Drupal/forum/Controller/ForumController.phpundefined
@@ -0,0 +1,144 @@
+   * Injects the ConfigFactory service to allow access to the forum settings.

@Inheritdoc

+++ b/core/modules/forum/lib/Drupal/forum/Controller/ForumController.phpundefined
@@ -0,0 +1,144 @@
+   * Constructor.

Should be "Constructs a ...". Additional I like it when __construct is first, so it is easier to see all the dependencies.

+++ b/core/modules/forum/lib/Drupal/forum/Controller/ForumController.phpundefined
@@ -0,0 +1,144 @@
+    $build['#attached']['css'][] = drupal_get_path('module', 'forum') . '/forum.css';
...
+    $build['#attached']['css'][] = drupal_get_path('module', 'forum') . '/forum.css';

Let's make a follow up to convert this to a library.

+++ b/core/modules/forum/lib/Drupal/forum/ForumBreadcrumbBuilder.phpundefined
@@ -47,31 +47,26 @@ public function __construct(EntityManager $entity_manager, ConfigFactory $config
+    if (isset($attributes['drupal_menu_item']) &&
+      ($item = $attributes['drupal_menu_item']) &&
+      $item['path'] == 'node/%') {
+
+      $node = $item['map'][1];
+      // Load the object in case of missing wildcard loaders.
+      $node = is_object($node) ? $node : node_load($node);
+      if (_forum_node_check_node_type($node)) {
+        $breadcrumb = $this->forumPostBreadcrumb($node);
       }

I think the proper way is to use $attributes['system_path']

+++ b/core/modules/forum/lib/Drupal/forum/ForumManager.phpundefined
@@ -0,0 +1,489 @@
+   * Overrides \Drupal\Core\Entity\DatabaseStorageController::resetCache().

Should be @inheritdoc

larowlan’s picture

Moves _forum_node_check_node_type() to the manager.

Status: Needs review » Needs work
Issue tags: -Needs architectural review, -Entity system, -Platform Initiative, -WSCCI-conversion

The last submitted patch, forum-page-route-1951268.34.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review

#35: forum-page-route-1951268.34.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Needs architectural review, +Entity system, +Platform Initiative, +WSCCI-conversion

The last submitted patch, forum-page-route-1951268.34.patch, failed testing.

larowlan’s picture

Status: Needs work » Needs review
FileSize
46.41 KB
5.06 KB

Fixed syntax errors in field.api.php (doh) and comments from #34 except

I think the proper way is to use $attributes['system_path']

That is the existing code (just moved out of an extra if statement) and will be replaced in #2026081: Clean up ForumBreadcrumbBuilder::build()

Follow up is #2028113: Make forum module css a library

larowlan’s picture

Adding an interface for the manager, this puts another bucketload of SQL queries behind a swappable service. Has to make pluggable backends more likely.

tim.plunkett’s picture

I like the architectural changes, here are some nitpicks.

+++ b/core/modules/forum/forum.moduleundefined
@@ -93,18 +94,11 @@ function forum_theme() {
-    'title callback' => 'entity_page_label',
-    'title arguments' => array(1),

Are you sure you want to ditch these? As of yesterday it will work again...

+++ b/core/modules/forum/forum.moduleundefined
@@ -247,18 +245,26 @@ function forum_uri($forum) {
+  foreach ($entities as $tid => &$forum_term) {

Is the & needed here? I don't think so.

+++ b/core/modules/forum/forum.moduleundefined
@@ -247,18 +245,26 @@ function forum_uri($forum) {
+      $forum_term->container = TRUE;
...
+  return $entities;

Can this just be $entity? Seeing the manipulation of $forum_term but returning $entities is confusing.

+++ b/core/modules/forum/forum.routing.ymlundefined
@@ -10,3 +10,15 @@ forum_settings:
   requirements:
     _permission: 'administer forums'
+forum_index:
...
+    _permission: 'access content'
+forum_page:
+  pattern: '/forum/{taxonomy_term}'

Let's put a newline in between each of these for readability

+++ b/core/modules/forum/lib/Drupal/forum/Controller/ForumController.phpundefined
@@ -0,0 +1,142 @@
+   * Config item.
+   *
+   * @var \Drupal\Core\Config\Config
+   */
+  protected $config;
...
+    $this->config = $config_factory->get('forum.settings');
...
+    $this->storageController = $storage_controller;
...
+      $container->get('config.factory'),
...
+      $container->get('plugin.manager.entity')->getStorageController('taxonomy_vocabulary')

So this is a bit odd. I'm used to seeing $this->configFactory stored, not the config object. And that'd be fine, but if you're going to not store the factory, I'd say do the ->get('forum.settings') in create() to keep the protected property and __construct() with the same typehint, and to perform that drilldown at the same place we do the storage controller.

+++ b/core/modules/forum/lib/Drupal/forum/Controller/ForumController.phpundefined
@@ -0,0 +1,142 @@
+    // Page title.
+    drupal_set_title($taxonomy_term->label());

Yeah this shouldn't be needed, see my earlier comment about entity_page_label()

+++ b/core/modules/forum/lib/Drupal/forum/Controller/ForumController.phpundefined
@@ -0,0 +1,142 @@
+    $build = array(
+      '#theme' => 'forums',
+      '#forums' => $taxonomy_term->forums,
+      '#topics' => $topics,
+      '#parents' => $taxonomy_term->parents,
+      '#tid' => $taxonomy_term->id(),
+      '#sortby' => $this->config->get('topics.order'),
+      '#forums_per_page' => $this->config->get('topics.page_limit'),
+    );
+    // @todo Make this a library - see https://drupal.org/node/2028113.
+    $build['#attached']['css'][] = drupal_get_path('module', 'forum') . '/forum.css';
+    return $build;
...
+    $build = array(
+      '#theme' => 'forums',
+      '#forums' => $index->forums,
+      '#topics' => '',
+      '#parents' => array(),
+      '#tid' => $index->id(),
+      '#sortby' => $this->config->get('topics.order'),
+      '#forums_per_page' => $this->config->get('topics.page_limit'),
+    );
+    // @todo Make this a library - see https://drupal.org/node/2028113.
+    $build['#attached']['css'][] = drupal_get_path('module', 'forum') . '/forum.css';
+    return $build;

Is this worth putting in a protected method?

+++ b/core/modules/forum/lib/Drupal/forum/Controller/ForumController.phpundefined
@@ -0,0 +1,142 @@
+    // Set the page title to forum's vocabulary name.
+    drupal_set_title($vocabulary->label());
+    if (empty($index->forums)) {
+      // Root of empty forum.
+      drupal_set_title(t('No forums defined'));

I think the first drupal_set_title should be in an else {}

+++ b/core/modules/forum/lib/Drupal/forum/ForumBreadcrumbBuilder.phpundefined
@@ -31,47 +32,52 @@ class ForumBreadcrumbBuilder implements BreadcrumbBuilderInterface {
+    if (isset($attributes['drupal_menu_item']) &&
+      ($item = $attributes['drupal_menu_item']) &&
+      $item['path'] == 'node/%') {
...
+    if (!empty($attributes['_route']) &&
+        $attributes['_route'] == 'forum_page' &&
+        isset($attributes['taxonomy_term'])) {

I think these are less readable than a single line...

+++ b/core/modules/forum/lib/Drupal/forum/ForumManager.phpundefined
@@ -0,0 +1,488 @@
+  /**
+   * Entity manager service
+   *
+   * @var \Drupal\Core\Entity\EntityManager
+   */
+  protected $entityManager;
...
+    $this->entityManager = $entity_manager;
...
+      $nodes = $this->entityManager->getStorageController('node')->load($nids);
...
+    $index = $this->entityManager->getStorageController('taxonomy_term')->create(array(

Let's inject the specific controllers here, especially since they have interfaces

+++ b/core/modules/forum/lib/Drupal/forum/ForumManager.phpundefined
@@ -0,0 +1,488 @@
+   * Database service
...
+  protected $database;

Not a service, and I think a lot of conversions are using $connection now.

+++ b/core/modules/forum/lib/Drupal/forum/ForumManager.phpundefined
@@ -0,0 +1,488 @@
+    $this->config = $config_factory->get('forum.settings');

Same thing about config vs config factory

+++ b/core/modules/forum/lib/Drupal/forum/ForumManager.phpundefined
@@ -0,0 +1,488 @@
+    $query = $this->database->select('forum_index', 'f')
+      ->extend('Drupal\Core\Database\Query\PagerSelectExtender')
+      ->extend('Drupal\Core\Database\Query\TableSortExtender');
+    $query->fields('f');
+    $query
+      ->condition('f.tid', $tid)
+      ->addTag('node_access')
+      ->addMetaData('base_table', 'forum_index')
+      ->orderBy('f.sticky', 'DESC')
+      ->orderByHeader($forum_topic_list_header)
+      ->limit($forum_per_page);

Not sure if all of this could use entity.query, but we should ask @dawehner, he can work magic with that (maybe a follow-up)

+++ b/core/modules/forum/lib/Drupal/forum/ForumManager.phpundefined
@@ -0,0 +1,488 @@
+   * Util method to ensure comment_num_new is behind a method.
...
+   * Utility method to get the last post information for the given forum tid.
...
+   * Utility method to fetch statistics for a forum.

+++ b/core/modules/forum/lib/Drupal/forum/ForumManagerInterface.phpundefined
@@ -0,0 +1,81 @@
+   * Utility method to fetch the child forums for a given forum.

These are supposed to start with a word ending in 's'

+++ b/core/modules/forum/lib/Drupal/forum/ForumManager.phpundefined
@@ -0,0 +1,488 @@
+   *   Node id.
...
+   *   The node ID.

I like "The node ID." better

+++ b/core/modules/forum/lib/Drupal/forum/ForumManager.phpundefined
@@ -0,0 +1,488 @@
+   * @return \stdClass|NULL

\stdClass|null when in a typehint.

+++ b/core/modules/forum/lib/Drupal/forum/ForumManager.phpundefined
@@ -0,0 +1,488 @@
+   *   Statistics for the given forum if statistics exist, else NULL

missing trailing period

+++ b/core/modules/forum/lib/Drupal/forum/ForumManagerInterface.phpundefined
@@ -0,0 +1,81 @@
+   *   Term id.
...
+   *   The forum vocabulary id.
...
+   *   The forum id to fetch the children for.
...
+   *   Term id.

ID, not id

+++ b/core/modules/forum/lib/Drupal/forum/ForumManagerInterface.phpundefined
@@ -0,0 +1,81 @@
+  public function checkNodeType(EntityInterface $node);

Can we use NodeInterface here?

tim.plunkett’s picture

A further note about $this->config vs $this->configFactory:

Assigning $this->config in __construct():
\Drupal\system\Form\DateFormatEditForm
\Drupal\forum\ForumBreadcrumbBuilder

Assigning $this->configFactory in __construct():
\Drupal\Core\Config\ConfigImporter
\Drupal\Core\Config\Entity\ConfigStorageController (and 11 and counting config entity controllers)
\Drupal\Core\Datetime\Date
\Drupal\aggregator\Controller\AggregatorController
\Drupal\system\SystemConfigFormBase (and 26 and counting config forms)
\Drupal\system\Form\DateFormatLocalizeResetForm
\Drupal\block\BlockListController
\Drupal\user\UserAutocomplete

nick_schuch’s picture

Assigned: larowlan » nick_schuch

Had a chat with larowlan, Ill work on these updates.

nick_schuch’s picture

Status: Needs review » Needs work
FileSize
47.67 KB
13.02 KB

The is my work so far. Still needs the injection changes.

larowlan’s picture

Makes the injections changes. We can't inject the storage controllers because this is a service, not a controller.
Interdiff and patch as well as combined interdiff of 44 and 45 for ease of reviewing.

Status: Needs review » Needs work

The last submitted patch, forum-page-route-1951268.45.patch, failed testing.

larowlan’s picture

Status: Needs work » Needs review
FileSize
47.79 KB
951 bytes

The interface type hint was updated but the ForumManager wasn't

Status: Needs review » Needs work

The last submitted patch, forum-page-route-1951268.47.patch, failed testing.

tim.plunkett’s picture

+++ b/core/modules/forum/lib/Drupal/forum/ForumManager.phpundefined
@@ -10,8 +10,8 @@
+use Drupal\node\Plugin\Core\Entity\NodeInterface;

Drupal\node\NodeInterface

larowlan’s picture

Status: Needs work » Needs review
FileSize
47.75 KB
1.36 KB

Doh

Status: Needs review » Needs work

The last submitted patch, forum-page-route-1951268.50.patch, failed testing.

larowlan’s picture

node_type_get_label() is gone.
Patch, interdiff and cumulative interdiff since last review.

andypost’s picture

Looks ready except couple of functions that left out of interface

+++ b/core/modules/forum/lib/Drupal/forum/ForumManager.phpundefined
@@ -0,0 +1,489 @@
+   * Provides the last post information for the given forum tid.
...
+  protected function getLastPost($tid) {
...
+   * Provides statistics for a forum.
...
+  protected function getForumStatistics($tid) {

Why this functions not described in interface?

andypost’s picture

Status: Needs review » Reviewed & tested by the community

This methods are protected so can't live in interface (thanx to @berdir)

YesCT’s picture

Issue tags: +RTBC July 1

This issue was RTBC and passing tests on July 1, the beginning of API freeze.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Needs reroll

git ac https://drupal.org/files/forum-page-route-1951268.52.patch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 48911  100 48911    0     0  21539      0  0:00:02  0:00:02 --:--:-- 31153
error: patch failed: core/modules/forum/forum.module:494
error: core/modules/forum/forum.module: patch does not apply
larowlan’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
47.78 KB
1.45 KB

re-roll

Status: Reviewed & tested by the community » Needs work
Issue tags: -Entity system, -Platform Initiative, -Needs reroll, -WSCCI-conversion, -RTBC July 1

The last submitted patch, forum-page-route-1951268.57.patch, failed testing.

larowlan’s picture

Status: Needs work » Needs review
Issue tags: +Entity system, +Platform Initiative, +Needs reroll, +WSCCI-conversion, +RTBC July 1

#57: forum-page-route-1951268.57.patch queued for re-testing.

larowlan’s picture

Status: Needs review » Reviewed & tested by the community

Back to rtbc

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/forum/lib/Drupal/forum/Controller/ForumController.phpundefined
@@ -0,0 +1,147 @@
+    $this->config = $config_factory->get('forum.settings');
...
+      $container->get('config.factory'),

Why bother injecting the whole factory if you're just going to use one config object from it?

+++ b/core/modules/forum/lib/Drupal/forum/Controller/ForumController.phpundefined
@@ -0,0 +1,147 @@
+      drupal_set_title(t('No forums defined'));

t can now be injected

+++ b/core/modules/forum/lib/Drupal/forum/ForumManager.phpundefined
@@ -0,0 +1,489 @@
+      array('data' => t('Topic'), 'field' => 'f.title'),
+      array('data' => t('Replies'), 'field' => 'f.comment_count'),
+      array('data' => t('Last reply'), 'field' => 'f.last_comment_timestamp'),

t can be injected

alexpott’s picture

larowlan pointed out in irc that you could say @timplunkett in #41 and I in #61 have said different things... but I disagree. Quoting tim

So this is a bit odd. I'm used to seeing $this->configFactory stored, not the config object. And that'd be fine, but if you're going to not store the factory, I'd say do the ->get('forum.settings') in create() to keep the protected property and __construct() with the same typehint, and to perform that drilldown at the same place we do the storage controller.

... I want to see the ->get('forum.settings') in create() too :) and have a protected $config typehinted to Config and not ConfigFactory...

larowlan’s picture

Thanks Alex, that makes sense now. Irc client dead on my phone :(

tim.plunkett’s picture

If we store configFactory, then we can retrieve any other config objects later without API changes. If we inject a single config object, any needs for another config object will cause an API change.

alexpott’s picture

I'm okay with injecting the config factory - it's no biggie - but using the logic of #64 shouldn't we be injecting the container to protect ourselves from making API changes (/me removes tongue from cheek).

fubhy’s picture

If we store configFactory, then we can retrieve any other config objects later without API changes. If we inject a single config object, any needs for another config object will cause an API change.

I don't really consider that an API change. So far (in other issues) we've been explicitly injecting the one config we need, not the entire factory. Not that it bothers me much, but I wonder why this controller is special and differs from the other WSCCI conversion issues we've re-rolled to explicitly inject the config only.

tim.plunkett’s picture

Well I didn't know about those issues. Every single conversion I've done or commented on has the factory.

grep -nr "this->configFactory = " core | wc -l
18

grep -nr " \$container->get('config.factory')->get(" core | wc -l
1
grep -nr "this->.* = \$config_factory->get" core
2

larowlan’s picture

larowlan’s picture

So made the changes asked for by @alexpott at #61 however there was a big merge conflict with #1951278: Create taxonomy term form controller for forum terms (ie not default) and use that for admin/structure/forum/edit/{forum|contain as to be expected so no interdiff.
But while I was at it, and seeing as though we are already moving the remaining private forum module functions (prefixed with _) into the manager, I also moved _forum_update_forum_index() and _forum_topics_unread() which means that once this and the last WSCCI conversion (#1974210: Convert admin/structure/forum to a Controller) for forum.module go in (drum-roll) forum module will contain no procedural functions other than hook implementations.
Yes this is an API change because its two extra function but they were considered internal functions anyway (the _ prefix has long been used as an indicator of pseudo-protected methods). And we were already removing a heap of them in this issue anyway (which was RTBC before Jul 1).
Happy to reverse that last interdiff if I'm being cheeky but I think this is a real chance for us to point to a module as 'done'. Also moving these into the manager helps the mongo case as it moves more hardcoded non EFQ queries behind a swappable service.
So pretty please? With sugar on top?

larowlan’s picture

Oh and I removed forum.pages.inc in this patch because its now empty (sweet)

larowlan’s picture

Status: Needs work » Needs review

doh

jibran’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC as #61 is addressed.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

I'm wondering whether or not ForumManager is doing too much and whether or not we should have a ForumStorageController that does all the db grunt work so that if someone whats to use some from nosql and the forum module it might be possible without having to rewrite too much of the business logic.

+++ b/core/modules/forum/forum.moduleundefined
@@ -254,18 +254,26 @@ function forum_uri($forum) {
+function forum_taxonomy_term_load(array $entities) {
+  $config = config('forum.settings');
+  $vid = $config->get('vocabulary');
+  foreach ($entities as $tid => $entity) {
+    if ($entity->bundle() != $vid) {
+      // Not a forum term.
+      continue;
+    }
+
+    // Determine if the requested term is a container.
+    if (in_array($entity->id(), $config->get('containers'))) {
+      $entity->container = TRUE;
+    }
+  }
+  return $entities;
 }

Whilst chatting to catch he brought up the issue of whether this approach is going to work in the EntityNG world.It think we need input from @berdir or @fago. Maybe Forum container need to become their own sort of entity?

Berdir’s picture

Attaching undefined stuff to EntityNG objects still works, with a few limitations, but it is obviously not available when introspecting the fields on an entity, which has some consequences:
- It's ignored when being serialized for REST (serialize() still works, atm)
- Entity form controller buildEntity() ignores it, we no longer put any form values on the entity object

So, if you e.g. want to be able to export a term entity and keep the information that it is a container, you need to define that stuff.

Which isn't too hard, see path_entity_field_info() for an example, you can also define something only for a specific bundle, see field_entity_field_info() for details about that. Note that when you do that, you need to access it as $term->container->value.

alexpott’s picture

Talked to @larowlan on IRC. In summary, the issue with making forum's container a fully paid up EntityNG property, deployable through rest, is that it will not work because the dependency between the taxonomy term and the config is maintained in ContainerFormController. So maybe we are best off where we are - I'm not sure.

With regards to "ForumManager is doing too much and whether or not we should have a ForumStorageController that does all the db grunt work". Thinking about this some more, I'll happy with this being attempted in a followup / D9. It's a lot of work to get right and not high priority considering you can't currently do this. So this patch does not change the current situation.

Leaving at "needs work" for @berdir and @larowlan to decide what do with ->container

larowlan’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
67.94 KB
11.53 KB

So discussed with @berdir and we decided that ->container should be a computed field.
So here it is.
Took a while to get it right, but I understand it now.
Also removed forum.pages.inc....because its now empty - win!
forum.admin.inc you're next

jibran’s picture

+++ b/core/modules/forum/lib/Drupal/forum/ForumContainerItem.phpundefined
@@ -0,0 +1,41 @@
+   * Implements \Drupal\Core\TypedData\ComplexDataInterface::getPropertyDefinitions().

+++ b/core/modules/forum/lib/Drupal/forum/ForumContainerValue.phpundefined
@@ -0,0 +1,56 @@
+   * Implements \Drupal\Core\TypedData\TypedDataInterface::getValue().
...
+   * Implements \Drupal\Core\TypedData\TypedDataInterface::setValue().

{@inheritdoc}

Status: Needs review » Needs work

The last submitted patch, forum-page-route-1951268.76.patch, failed testing.

larowlan’s picture

Status: Needs work » Needs review
FileSize
68.01 KB
2.64 KB

Fixes #77 and the failing forum tests.

kim.pepper’s picture

So many reviews already. The only thing I could come up with was:

+++ b/core/modules/forum/lib/Drupal/forum/ForumContainerValue.phpundefined
@@ -27,30 +27,32 @@ public function getValue() {
+      $field = $this->parent->getParent();

The $field variable seems redundant.

larowlan’s picture

    $field = $this->parent->getParent();
     $entity = $field->getParent();

Thanks, its used on the next line.

larowlan’s picture

oh you mean inline it :)
I've followed the convention used in eg CommentNewItem.

jibran’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC as #73-1 is dropped in #75 and #73-2 is addressed in #76. Let's kill forum.pages.inc.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/forum/lib/Drupal/forum/ForumManager.phpundefined
@@ -0,0 +1,552 @@
+    if ($this->index) {
+      return $index;
+    }

Looks like this should be return $this->index;. And the fact this is not producing a fail implies missing test coverage.

Couldn't last_post also be a computed field on a forum term? That we we would only calculate it if necessary.

+++ b/core/modules/forum/forum.moduleundefined
@@ -449,14 +433,16 @@ function forum_permission() {
+    $config = config('forum.settings');
+    $containers = $config->get('containers');
+    $key = array_search($term->id(), $containers);
+    if ($key !== FALSE) {
+      unset($containers[$key]);
+    }
+    $config->set('containers', $containers)->save();

We should only be doing the set and save if we do the unset.

And the patch needs a reroll

git applyc https://drupal.org/files/forum-page-route-1951268.79.patch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 69645  100 69645    0     0  20300      0  0:00:03  0:00:03 --:--:-- 21880
error: patch failed: core/modules/forum/lib/Drupal/forum/ForumBreadcrumbBuilder.php:31
error: core/modules/forum/lib/Drupal/forum/ForumBreadcrumbBuilder.php: patch does not apply

This is only a light review - I have not been through it line by line yet.

alexpott’s picture

Issue tags: +Needs tests

tagging due to

And the fact this is not producing a fail implies missing test coverage.

larowlan’s picture

Thanks @alexpott all great catches

alexpott’s picture

+++ b/core/modules/forum/lib/Drupal/forum/ForumManagerInterface.phpundefined
@@ -0,0 +1,104 @@
+use Drupal\node\NodeInterface;
...
+   * @param \Drupal\node\Plugin\Core\Entity\NodeInterface $node

Doesn't match the use statement - should be \Drupal\node\NodeInterface

larowlan’s picture

And the fact this is not producing a fail implies missing test coverage.

Its a dead code path in core alone.
But now we have a manager, we can add a UnitTest (win) for that code path.

larowlan’s picture

+++ b/core/modules/forum/lib/Drupal/forum/ForumManager.phpundefined
@@ -0,0 +1,552 @@
+  function updateIndex($nid) {

+++ b/core/modules/forum/lib/Drupal/forum/ForumManagerInterface.phpundefined
@@ -0,0 +1,104 @@
+  function updateIndex($nid);

Needs the public scope modifier

Also updating the issue summary

larowlan’s picture

Issue tags: +API change

Adding API Change

larowlan’s picture

Issue summary: View changes

Updated issue summary.

larowlan’s picture

And the constants need to go still.

larowlan’s picture

Issue summary: View changes

Updated api changes

larowlan’s picture

There are no existing constants, updating issue summary.

larowlan’s picture

Issue summary: View changes

Updated issue summary.

larowlan’s picture

Status: Needs work » Needs review
FileSize
70.99 KB
6.45 KB

Merges HEAD
Fixes #80 comments from @kim.pepper
Fixes #87 from @alexpott
Fixes #84 from @alexpott except

Couldn't last_post also be a computed field on a forum term? That we we would only calculate it if necessary.

I've spoken with @berdir about this and we'd be better to refactor the theme function, but thats an API change. Need time to mull over it some more.

jibran’s picture

Assigned: nick_schuch » Unassigned
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

Back to RTBC as all the feedback is fixed. Now it has a new test. Let's see what @alexpott suggest on api change.

larowlan’s picture

Issue tags: +PHPUnit

adds tag

star-szr’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Needs a reroll.

Checking patch core/modules/field/field.api.php...
Checking patch core/modules/forum/forum.admin.inc...
Checking patch core/modules/forum/forum.module...
error: while searching for:

use Drupal\Core\Entity\EntityInterface;
use Drupal\entity\Plugin\Core\Entity\EntityDisplay;
use Drupal\taxonomy\Plugin\Core\Entity\Term;

/**

error: patch failed: core/modules/forum/forum.module:7
error: core/modules/forum/forum.module: patch does not apply
Checking patch core/modules/forum/forum.pages.inc...
error: while searching for:
<?php

/**
 * @file
 * User page callbacks for the Forum module.
 */

/**
 * Page callback: Prints a forum listing.
 *
 * @param $forum_term
 *   A tree of all forums for a given taxonomy term ID. Defaults to NULL. See
 *   the return object of forum_forum_load() for a complete definition.
 *
 * @return
 *   A string containing HTML representing the themed forum listing.
 *
 * @see forum_menu()
 */
function forum_page($forum_term = NULL) {
  $config = config('forum.settings');
  $vocabulary = entity_load('taxonomy_vocabulary', $config->get('vocabulary'));

  if (!isset($forum_term)) {
    // On the main page, display all the top-level forums.
    $forum_term = forum_forum_load(0);
    // Set the page title to forum's vocabulary name.
    drupal_set_title($vocabulary->label());
  }

  if ($forum_term->id() && array_search($forum_term->id(), $config->get('containers')) === FALSE) {
    // Add RSS feed for forums.
    drupal_add_feed('taxonomy/term/' . $forum_term->id() . '/feed', 'RSS - ' . $forum_term->label());
  }

  if (empty($forum_term->forums) && empty($forum_term->parents)) {
    // Root of empty forum.
    drupal_set_title(t('No forums defined'));
  }

  $forum_per_page = $config->get('topics.page_limit');
  $sort_by = $config->get('topics.order');

  if (empty($forum_term->container)) {
    $topics = forum_get_topics($forum_term->id(), $sort_by, $forum_per_page);
  }
  else {
    $topics = '';
  }

  $build = array(
    '#theme' => 'forums',
    '#forums' => $forum_term->forums,
    '#topics' => $topics,
    '#parents' => $forum_term->parents,
    '#tid' => $forum_term->id(),
    '#sortby' => $sort_by,
    '#forums_per_page' => $forum_per_page,
  );
  $build['#attached']['css'][] = drupal_get_path('module', 'forum') . '/css/forum.module.css';
  return $build;
}

error: patch failed: core/modules/forum/forum.pages.inc:1
error: core/modules/forum/forum.pages.inc: patch does not apply
Checking patch core/modules/forum/forum.routing.yml...
Checking patch core/modules/forum/forum.services.yml...
Checking patch core/modules/forum/lib/Drupal/forum/Controller/ForumController.php...
Checking patch core/modules/forum/lib/Drupal/forum/ForumBreadcrumbBuilder.php...
Checking patch core/modules/forum/lib/Drupal/forum/ForumContainerItem.php...
Checking patch core/modules/forum/lib/Drupal/forum/ForumContainerValue.php...
Checking patch core/modules/forum/lib/Drupal/forum/ForumManager.php...
Checking patch core/modules/forum/lib/Drupal/forum/ForumManagerInterface.php...
Checking patch core/modules/forum/lib/Drupal/forum/Tests/ForumTest.php...
Checking patch core/modules/forum/tests/Drupal/forum/Tests/ForumManagerTest.php...
larowlan’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
71.03 KB

straight reroll, back to RTBC as per #94

catch’s picture

Coming into this really late, but why not have container as a boolean configurable field on taxonomy terms - can just be applied to the forum term bundle. Seems like we're going to a lot of trouble to keep it in the configuration system which was a straight port from the variable (which has been around since probably 4.5/6 or earlier).

larowlan’s picture

Yes this could be done, do you think its in scope here or a follow-up?

larowlan’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
21.01 KB
23.94 KB
23.23 KB
23.82 KB
84.47 KB
27.16 KB

Here it is, its a big interdiff :)

Added new field, this means we need to change the signature of theme('forums') to use term instead of tid, as we can't make the comparison of tids against the containers CMI setting.

Added upgrade path and test.

Added new tests for field functionality.

Also found and fixed a styling issue (css file moved, change lost in merge).

Also found and fixed a warning relating to table drag on the index page. This still uses globals which is yuck.

Screenshots (note data is different but functionality remains intact):

Before

before-index.png

before-page.png

After

after-index.png

after-page.png

larowlan’s picture

Updated issue summary

larowlan’s picture

Issue summary: View changes

Updated issue summary.

Status: Needs review » Needs work

The last submitted patch, forum-page-route-1951268.100.patch, failed testing.

larowlan’s picture

Status: Needs work » Needs review
FileSize
85.37 KB
1.3 KB

Fixes test fails.

larowlan’s picture

Oh and #100 fixed an upgrade path bug in forum's CMI integration, we were transposing the vid into the forum.settings.vocabulary setting, should have been the machine name. Found it while debugging the new upgrade path tests specific for forum. Yay for upgrade tests.

jibran’s picture

Issue tags: -Needs reroll

tbh I dono what is going on after #98. But I think we should add options module under dependencies in forum.info.yml

larowlan’s picture

I did, but we need to make sure the field info type cache is primed before the default config is installed

larowlan’s picture

+++ b/core/modules/forum/forum.info.yml
@@ -6,6 +6,7 @@ dependencies:
+  - options

from interdiff at #100

catch’s picture

Wow that was quick.

Interdiff looks great.

+  // Load the dependent Options module, in case it has been disabled.
+  drupal_load('module', 'options');
+
+  if ($field = field_info_field('forum_container')) {
+    $field->delete();
+  }

This should be handled by adding options as a dependency? I noticed a couple of minor comment issues but otherwise seems good to me.

larowlan’s picture

I think without it we fail the ModuleEnableDisableTest because of the whole 'disabled modules are broken beyond repair', field_info_field doesn't recognise disabled module fields (these are in state). Posted a patch without it here #1958050-303: IGNORE: Patch testing issue ✌✌, let's see how it goes

larowlan’s picture

Test passed, so yep, don't need the load.

larowlan’s picture

Green again, anyone for a review?

dawehner’s picture

  1. +++ b/core/modules/forum/forum.install
    @@ -118,8 +133,15 @@ function forum_uninstall() {
    +  if ($field = field_info_field('forum_container')) {
    

    This method is already marked as deprecated

  2. +++ b/core/modules/forum/forum.routing.yml
    @@ -4,30 +4,49 @@ forum_delete:
         _permission: 'administer forums'
    +
     forum_add_forum:
       pattern: 'admin/structure/forum/add/forum'
       defaults:
         _content: 'Drupal\forum\Controller\ForumController::addForum'
       requirements:
         _permission: 'administer forums'
    +
     forum_edit_container:
       pattern: 'admin/structure/forum/edit/container/{taxonomy_term}'
       defaults:
         _entity_form: 'taxonomy_term.container'
       requirements:
         _permission: 'administer forums'
    +
     forum_edit_forum:
       pattern: 'admin/structure/forum/edit/forum/{taxonomy_term}'
    

    Let's try to skip out of scope changes.

  3. +++ b/core/modules/forum/lib/Drupal/forum/Controller/ForumController.php
    @@ -33,37 +44,140 @@ class ForumController implements ControllerInterface {
    +      // Root of empty forum.
    +      drupal_set_title($this->translationManager->translate('No forums defined'));
    ...
    +      // Set the page title to forum's vocabulary name.
    +      drupal_set_title($vocabulary->label());
    

    Let's use '#title' for now, as part of the returning render array.

  4. +++ b/core/modules/forum/lib/Drupal/forum/ForumManager.php
    @@ -0,0 +1,552 @@
    +   * @var \Drupal\Core\StringTranslation\TranslationManager
    ...
    +   * @param \Drupal\Core\StringTranslation\TranslationManager $translation_manager
    ...
    +  public function __construct(ConfigFactory $config_factory, EntityManager $entity_manager, Connection $connection, FieldInfo $field_info, TranslationManager $translation_manager) {
    

    Afaik we do have interface now.

  5. +++ b/core/modules/forum/lib/Drupal/forum/ForumManager.php
    @@ -0,0 +1,552 @@
    +    $query = $this->connection->select('node_field_data', 'n');
    ...
    +  public function updateIndex($nid) {
    +    $count = $this->connection->query('SELECT COUNT(cid) FROM {comment} c INNER JOIN {forum_index} i ON c.nid = i.nid WHERE c.nid = :nid AND c.status = :status', array(
    ...
    +      $last_reply = $this->connection->queryRange('SELECT cid, name, created, uid FROM {comment} WHERE nid = :nid AND status = :status ORDER BY cid DESC', 0, 1, array(
    ...
    +      $node = $this->connection->query('SELECT uid, created FROM {node_field_data} WHERE nid = :nid AND default_langcode = 1', array(':nid' => $nid))->fetchObject();
    

    Is there no other way then directly querying the node_field_data/comment table? Can't we use EQ somehow?

  6. +++ b/core/modules/forum/lib/Drupal/forum/ForumManagerInterface.php
    @@ -0,0 +1,104 @@
    +   * @return \Drupal\taxonomy\Plugin\Core\Entity\Term
    

    Should we rather return the term interface?

  7. +++ b/core/modules/forum/lib/Drupal/forum/ForumManagerInterface.php
    @@ -0,0 +1,104 @@
    +   * @param integer $term
    ...
    +   * @param integer $uid
    ...
    +  public function updateIndex($nid);
    

    Afaik we are using int not integer, see https://drupal.org/node/1354

  8. +++ b/core/modules/forum/tests/Drupal/forum/Tests/ForumManagerTest.php
    @@ -0,0 +1,101 @@
    +class ForumManagerTest extends UnitTestCase {
    

    nice!

  9. +++ b/core/modules/system/lib/Drupal/system/Tests/Upgrade/ForumUpgradePathTest.php
    @@ -0,0 +1,58 @@
    + * Definition of Drupal\system\Tests\Upgrade\ForumUpgradePathTest.
    

    Contains \....

larowlan’s picture

Fixes all of #112 except 5, we can't for any of those queries because they join one of forum_index or node_comment_statistics. However, as discussed on irc, the ForumManager wraps these queries so we can swap in another backend.

Status: Needs review » Needs work

The last submitted patch, forum-page-route-1951268.113.patch, failed testing.

larowlan’s picture

Status: Needs work » Needs review
FileSize
84.71 KB
1.04 KB

Node BC decorator is gone

larowlan’s picture

Back to green

larowlan’s picture

larowlan’s picture

Issue summary: View changes

Updated issue summary.

larowlan’s picture

Added #2077435: Use a yml file to create the forum vocabulary
Removed the yml addition from this issue, scope creep

jibran’s picture

  1. +++ b/core/modules/forum/lib/Drupal/forum/Controller/ForumController.php
    @@ -33,37 +44,141 @@ class ForumController implements ControllerInterface {
    +   * @param \Drupal\Core\Config\Config $config
    +   *   The config factory.
    ...
    +  public function __construct(Config $config, ForumManagerInterface $forum_manager, VocabularyStorageControllerInterface $vocabulary_storage_controller, TermStorageControllerInterface $term_storage_controller, EntityManager $entity_manager, TranslationManager $translation_manager) {
    ...
    +      $container->get('config.factory')->get('forum.settings'),
    

    Just a question, is $config still swapable? This should be "The forum config factory" I think.

  2. +++ b/core/modules/forum/lib/Drupal/forum/ForumBreadcrumbBuilder.php
    @@ -10,6 +10,7 @@
    @@ -31,47 +32,47 @@ class ForumBreadcrumbBuilder implements BreadcrumbBuilderInterface {
    

    Do we already have test for this in core?

  3. +++ b/core/modules/forum/lib/Drupal/forum/ForumBreadcrumbBuilder.php
    @@ -31,47 +32,47 @@ class ForumBreadcrumbBuilder implements BreadcrumbBuilderInterface {
    +    if (!empty($attributes['_route']) && $attributes['_route'] == 'forum_page' && isset($attributes['taxonomy_term'])) {
    

    @see #1978958-100: Convert comment_reply() to a Controller

  4. +++ b/core/modules/forum/lib/Drupal/forum/ForumManager.php
    @@ -0,0 +1,552 @@
    +      array('data' => t('Topic'), 'field' => 'f.title'),
    +      array('data' => t('Replies'), 'field' => 'f.comment_count'),
    +      array('data' => t('Last reply'), 'field' => 'f.last_comment_timestamp'),
    

    We have $this->translationManager.

  5. +++ b/core/modules/forum/lib/Drupal/forum/ForumManager.php
    @@ -0,0 +1,552 @@
    +          // @todo move use comment service.
    

    Please update the comment. I am unable to understand it.

  6. +++ b/core/modules/forum/lib/Drupal/forum/ForumManager.php
    @@ -0,0 +1,552 @@
    +      'container' => TRUE,
    

    I think this should be 1 instead of TRUE.

  7. +++ b/core/modules/forum/lib/Drupal/forum/ForumManager.php
    @@ -0,0 +1,552 @@
    +      // @todo This should be actually filtering on the desired node language and
    

    80 char limit.

  8. +++ b/core/modules/forum/lib/Drupal/forum/ForumManagerInterface.php
    @@ -0,0 +1,104 @@
    +interface ForumManagerInterface {
    

    Is there any reason why we have not added all the ForumManger methods to interface?

larowlan’s picture

1-7 thanks will fix
8, all public methods are, protected don't go on the interface, see comments above

larowlan’s picture

1-7 fixed except 2, we already have coverage
8 see #120
re 5, just removed, there is no such service until #731724: Convert comment settings into a field to make them work with CMI and non-node entities goes in

jibran’s picture

Status: Needs review » Reviewed & tested by the community

@catch was happy in #108. I am unable to find any other issue so back to RTBC.

larowlan’s picture

Please give the reviewers some commit credit here, its been a long process :)

larowlan’s picture

Issue summary: View changes

Updated issue summary.

larowlan’s picture

Issue summary: View changes

Updated issue summary.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Needs reroll

git ac https://drupal.org/files/forum-page-route-1951268.121.patch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 86484  100 86484    0     0  47516      0  0:00:01  0:00:01 --:--:-- 52702
error: patch failed: core/modules/field/field.api.php:1117
error: core/modules/field/field.api.php: patch does not apply
error: patch failed: core/modules/forum/forum.module:168
error: core/modules/forum/forum.module: patch does not apply
error: patch failed: core/modules/forum/lib/Drupal/forum/Controller/ForumController.php:7
error: core/modules/forum/lib/Drupal/forum/Controller/ForumController.php: patch does not apply
larowlan’s picture

Status: Needs work » Needs review
FileSize
81.35 KB

reroll

Status: Needs review » Needs work
Issue tags: -Entity system, -PHPUnit, -API change, -Platform Initiative, -Needs reroll, -WSCCI-conversion, -RTBC July 1

The last submitted patch, forum-page-route-1951268.reroll.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
Issue tags: +Entity system, +PHPUnit, +API change, +Platform Initiative, +Needs reroll, +WSCCI-conversion, +RTBC July 1
Berdir’s picture

Testbot was broken, trying again.

Status: Needs review » Needs work

The last submitted patch, forum-page-route-1951268.reroll.patch, failed testing.

larowlan’s picture

Status: Needs work » Needs review
FileSize
81.31 KB

Status: Needs review » Needs work

The last submitted patch, forum-page-route-1951268.reroll2.patch, failed testing.

star-szr’s picture

I could be wrong but it looks like some hunks were lost after #121, just diffed the patches.

larowlan’s picture

Status: Needs work » Needs review
FileSize
81.25 KB
792 bytes

Format of field yml changed

Status: Needs review » Needs work

The last submitted patch, forum-page-route-1951268.132.patch, failed testing.

larowlan’s picture

Status: Needs work » Needs review
FileSize
81.26 KB
1.49 KB

Tables names changed in #1497374: Switch from Field-based storage to Entity-based storage so upgrade path failed.
Plus needed a field name and different id.

Status: Needs review » Needs work

The last submitted patch, forum-page-route-1951268.134.patch, failed testing.

larowlan’s picture

Status: Needs work » Needs review
FileSize
81.27 KB
1.04 KB

missing a use and wrong use of entity_load for the field

stupidest. reroll. ever.

Status: Needs review » Needs work

The last submitted patch, forum-page-route-1951268.136.patch, failed testing.

larowlan’s picture

This time. please. really.
A new call to _forum_update_forum_index() was added in #1497374: Switch from Field-based storage to Entity-based storage

jibran’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll

O come on. July 1st huh. Sept. 1st is gone now :(

alexpott’s picture

Re #123 and please give the reviewers some credit can you added suggested commit message please :)

larowlan’s picture

Perhaps:
Issue #1951268 by larowlan, nick_schuch, jibran, dawehner, alexpott, tim.plunkett: Convert /forum and /forum/% to new routing system, remove forum_forum_load(), forum_get_topics(), create new Forum service.

jibran’s picture

Status: Needs review » Reviewed & tested by the community

And back to RTBC.

jibran’s picture

Issue summary: View changes

Updated issue summary.

alexpott’s picture

Title: Convert /forum and /forum/% to new routing system, remove forum_forum_load(), forum_get_topics(), create new Forum service » Change notice: Convert /forum and /forum/% to new router, remove forum_forum_load(), forum_get_topics(), create Forum service
Priority: Normal » Critical
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

Committed 88b6c93 and pushed to 8.x. Thanks!

Fixed the following in commit

diff --git a/core/modules/forum/forum.module b/core/modules/forum/forum.module
index 0b5df66..0f076c4 100644
--- a/core/modules/forum/forum.module
+++ b/core/modules/forum/forum.module
@@ -8,7 +8,6 @@
 use Drupal\Core\Entity\EntityInterface;
 use Drupal\field\Field;
 use Drupal\node\NodeInterface;
-use Drupal\taxonomy\Entity\Term;
 
 /**
  * Implements hook_help().
@@ -383,7 +382,7 @@ function forum_node_update(EntityInterface $node) {
       }
       $query->execute();
       // The logic for determining last_comment_count is fairly complex, so
-      // call update the index too.
+      // update the index too.
       Drupal::service('forum_manager')->updateIndex($node->id());
     }
     // When a forum node is unpublished, remove it from the forum_index table.
diff --git a/core/modules/forum/lib/Drupal/forum/ForumManagerInterface.php b/core/modules/forum/lib/Drupal/forum/ForumManagerInterface.php
index 6072d87..b8896ce 100644
--- a/core/modules/forum/lib/Drupal/forum/ForumManagerInterface.php
+++ b/core/modules/forum/lib/Drupal/forum/ForumManagerInterface.php
@@ -41,10 +41,10 @@ public function getChildren($vid, $tid);
   /**
    * Generates and returns the forum index.
    *
-   * The forum index is a psuedo term that provides an overview of all forums.
+   * The forum index is a pseudo term that provides an overview of all forums.
    *
    * @return \Drupal\taxonomy\TermInterface
-   *   A psuedo term representing the overview of all forums.
+   *   A pseudo term representing the overview of all forums.
    */
   public function getIndex();
 
diff --git a/core/modules/system/lib/Drupal/system/Tests/Upgrade/ForumUpgradePathTest.php b/core/modules/system/lib/Drupal/system/Tests/Upgrade/ForumUpgradePathTest.php
index e1a50e4..5edc67c 100644
--- a/core/modules/system/lib/Drupal/system/Tests/Upgrade/ForumUpgradePathTest.php
+++ b/core/modules/system/lib/Drupal/system/Tests/Upgrade/ForumUpgradePathTest.php
@@ -7,8 +7,6 @@
 
 namespace Drupal\system\Tests\Upgrade;
 
-use Drupal\Core\Language\Language;
-
 /**
  * Tests upgrading a filled database with forum data.
  *
alexpott’s picture

Issue tags: +Approved API change

Oops forgot to add the tag... mind you this was RTBC July 1

ParisLiakos’s picture

larowlan’s picture

Status: Active » Needs review

Draft change notice here https://drupal.org/node/2079767

andypost’s picture

Title: Change notice: Convert /forum and /forum/% to new router, remove forum_forum_load(), forum_get_topics(), create Forum service » Convert /forum and /forum/% to new router, remove forum_forum_load(), forum_get_topics(), create Forum service
Priority: Critical » Normal
Status: Needs review » Fixed
Issue tags: -Needs change record

Very detailed change notice!

jibran’s picture

Yay!!! Thanks everyone great work :). Special thanks to @alexpott for approving the api change and committing it ;).

andypost’s picture

andypost’s picture

And another follow-up #2081629: Optimize ForumManager
PS lastVisit() & unreadTopics() fetches to much data

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

Anonymous’s picture

Issue summary: View changes

Add suggested commit message