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
Related Issues
- #1951274: Convert /forum and /forum/% to use an entity render controller
- #1951278: Create taxonomy term form controller for forum terms (ie not default) and use that for admin/structure/forum/edit/{forum|contain
- #2061997: Replace user_access() calls with $account->hasPermission() in forum module Assigned to: slimbk - postponed on this
- #2077435: Use a yml file to create the forum vocabulary - postponed on this
Comment | File | Size | Author |
---|---|---|---|
#139 | forum-page-route-1951268.139.interdiff.txt | 667 bytes | larowlan |
#139 | forum-page-route-1951268.139.patch | 81.74 KB | larowlan |
#137 | forum-page-route-1951268.136.interdiff.txt | 1.04 KB | larowlan |
#137 | forum-page-route-1951268.136.patch | 81.27 KB | larowlan |
#135 | forum-page-route-1951268.134.interdiff.txt | 1.49 KB | larowlan |
Comments
Comment #1
larowlanThe 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).
Comment #1.0
larowlanUpdated issue summary.
Comment #2
andypostAwesome! The only thing here that needs deeper review is usage of term-reference fields on forum nodes.
not sure it's good to set title from controller
same here
Comment #3
larowlanYes 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.
Comment #4
andypostAlso 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
Comment #5
Crell CreditAttribution: Crell commentedI *love* this concept. :-)
Needs a @return.
forum_get_topics() looks like it can be service-ified, or else made into a method of one of these classes.
This should be using an injected connection and $this->database->select().
Ibid.
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.
As it should be!
Comment #6
amateescu CreditAttribution: amateescu commentedWhoa.. 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 needs to happen for all entity storage controllers, and I don't think this is the issue that needs to deal with it.
Comment #7
larowlanThis should wait for #1818560: Convert taxonomy entities to the new Entity Field API
Comment #8
ParisLiakos CreditAttribution: ParisLiakos commentedVery nice..seems i should do something like that in #15266: Replace aggregator category system with taxonomy ?
Comment #9
yched CreditAttribution: yched commentedWondering 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) ?
Comment #10
fubhy CreditAttribution: fubhy commentedI 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).
Comment #11
andypostSuppose 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
Comment #12
BerdirTrying 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.
Comment #13
larowlanAn update on how we're going to tackle this in Drupal 8:
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.
Comment #13.0
larowlanUpdated issue summary.
Comment #14
larowlanUpdated issue summary, also this is definitely postponed pending #1818560: Convert taxonomy entities to the new Entity Field API
Comment #15
larowlanBack on this
Comment #16
larowlanSo this is a fairly major patch as follows:
Comment #18
larowlan#16: forum-page-route-1951268.16.patch queued for re-testing.
Comment #20
larowlanOne fail is the HEAD fail (since fixed)
The other was missing a \ on new \StdClass()
Comment #21
dawehnerIs this change intended? I am confused that there is title and title callback
Can we just skip this parts ... there is an issue for that
It would be possible to directly use Drupal::config()
What about calling "Field::fieldInfo()->getField()" directly?
Isn't it possible to just have a single route with ~ as default value?
public or protected?
Comment #22
larowlanYeah entity_page_label doesn't work with routes.
We added a drupal_set_title() instead.
Fixed rest of suggestions.
Comment #24
larowlan#22: forum-page-route-1951268.22.patch queued for re-testing.
Comment #25
Crell CreditAttribution: Crell commentedThe 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. :-)
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!
Comment #26
jibranRelated #2026081: Clean up ForumBreadcrumbBuilder::build()
Comment #27
jibranThis should be TermInterface
#1947536: Convert drupal_get_breadcrumb() and drupal_set_breadcrumb() to a service is committed now we can inject
forum.breadcrumb
Comment #28
Crell CreditAttribution: Crell commentedNo need to inject forum.breadcrumb. That code just goes away entirely as it's now handled independently by the breadcrumb service.
Comment #29
larowlanI'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.
Comment #31
andypostHow this affect usage of uri(), @larowlan please chime in #2010132: Canonical taxonomy term link for forum vocabulary is broken
Comment #32
larowlanMissed 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!
Comment #33
jibranCan we move
_forum_node_check_node_type
to manger?Comment #34
dawehnerSome additional points.
Missing starting "\"
These lines are redundant.
@Inheritdoc
Should be "Constructs a ...". Additional I like it when __construct is first, so it is easier to see all the dependencies.
Let's make a follow up to convert this to a library.
I think the proper way is to use $attributes['system_path']
Should be @inheritdoc
Comment #35
larowlanMoves _forum_node_check_node_type() to the manager.
Comment #37
dawehner#35: forum-page-route-1951268.34.patch queued for re-testing.
Comment #39
larowlanFixed syntax errors in field.api.php (doh) and comments from #34 except
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
Comment #40
larowlanAdding an interface for the manager, this puts another bucketload of SQL queries behind a swappable service. Has to make pluggable backends more likely.
Comment #41
tim.plunkettI like the architectural changes, here are some nitpicks.
Are you sure you want to ditch these? As of yesterday it will work again...
Is the & needed here? I don't think so.
Can this just be $entity? Seeing the manipulation of $forum_term but returning $entities is confusing.
Let's put a newline in between each of these for readability
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.
Yeah this shouldn't be needed, see my earlier comment about entity_page_label()
Is this worth putting in a protected method?
I think the first drupal_set_title should be in an
else {}
I think these are less readable than a single line...
Let's inject the specific controllers here, especially since they have interfaces
Not a service, and I think a lot of conversions are using $connection now.
Same thing about config vs config factory
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)
These are supposed to start with a word ending in 's'
I like "The node ID." better
\stdClass|null when in a typehint.
missing trailing period
ID, not id
Can we use NodeInterface here?
Comment #42
tim.plunkettA 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
Comment #43
nick_schuch CreditAttribution: nick_schuch commentedHad a chat with larowlan, Ill work on these updates.
Comment #44
nick_schuch CreditAttribution: nick_schuch commentedThe is my work so far. Still needs the injection changes.
Comment #45
larowlanMakes 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.
Comment #47
larowlanThe interface type hint was updated but the ForumManager wasn't
Comment #49
tim.plunkettDrupal\node\NodeInterface
Comment #50
larowlanDoh
Comment #52
larowlannode_type_get_label() is gone.
Patch, interdiff and cumulative interdiff since last review.
Comment #53
andypostLooks ready except couple of functions that left out of interface
Why this functions not described in interface?
Comment #54
andypostThis methods are protected so can't live in interface (thanx to @berdir)
Comment #55
YesCT CreditAttribution: YesCT commentedThis issue was RTBC and passing tests on July 1, the beginning of API freeze.
Comment #56
alexpottNeeds reroll
Comment #57
larowlanre-roll
Comment #59
larowlan#57: forum-page-route-1951268.57.patch queued for re-testing.
Comment #60
larowlanBack to rtbc
Comment #61
alexpottWhy bother injecting the whole factory if you're just going to use one config object from it?
t can now be injected
t can be injected
Comment #62
alexpottlarowlan pointed out in irc that you could say @timplunkett in #41 and I in #61 have said different things... but I disagree. Quoting tim
... I want to see the ->get('forum.settings') in create() too :) and have a protected $config typehinted to Config and not ConfigFactory...
Comment #63
larowlanThanks Alex, that makes sense now. Irc client dead on my phone :(
Comment #64
tim.plunkettIf 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.
Comment #65
alexpottI'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).
Comment #66
fubhy CreditAttribution: fubhy commentedI 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.
Comment #67
tim.plunkettWell 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
Comment #68
larowlanAre we injecting t() yet? I was waiting on #2018411: Figure out a nice DX when working with injected translation
Comment #69
larowlanSo 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?
Comment #70
larowlanOh and I removed forum.pages.inc in this patch because its now empty (sweet)
Comment #71
larowlandoh
Comment #72
jibranBack to RTBC as #61 is addressed.
Comment #73
alexpottI'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.
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?
Comment #74
BerdirAttaching 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.
Comment #75
alexpottTalked 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
Comment #76
larowlanSo 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
Comment #77
jibran{@inheritdoc}
Comment #79
larowlanFixes #77 and the failing forum tests.
Comment #80
kim.pepperSo many reviews already. The only thing I could come up with was:
The $field variable seems redundant.
Comment #81
larowlanThanks, its used on the next line.
Comment #82
larowlanoh you mean inline it :)
I've followed the convention used in eg CommentNewItem.
Comment #83
jibranBack to RTBC as #73-1 is dropped in #75 and #73-2 is addressed in #76. Let's kill
forum.pages.inc
.Comment #84
alexpottLooks 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.
We should only be doing the set and save if we do the unset.
And the patch needs a reroll
This is only a light review - I have not been through it line by line yet.
Comment #85
alexpotttagging due to
Comment #86
larowlanThanks @alexpott all great catches
Comment #87
alexpottDoesn't match the use statement - should be \Drupal\node\NodeInterface
Comment #88
larowlanIts a dead code path in core alone.
But now we have a manager, we can add a UnitTest (win) for that code path.
Comment #89
larowlanNeeds the public scope modifier
Also updating the issue summary
Comment #90
larowlanAdding API Change
Comment #90.0
larowlanUpdated issue summary.
Comment #91
larowlanAnd the constants need to go still.
Comment #91.0
larowlanUpdated api changes
Comment #92
larowlanThere are no existing constants, updating issue summary.
Comment #92.0
larowlanUpdated issue summary.
Comment #93
larowlanMerges HEAD
Fixes #80 comments from @kim.pepper
Fixes #87 from @alexpott
Fixes #84 from @alexpott except
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.
Comment #94
jibranBack to RTBC as all the feedback is fixed. Now it has a new test. Let's see what @alexpott suggest on api change.
Comment #95
larowlanadds tag
Comment #96
star-szrNeeds a reroll.
Comment #97
larowlanstraight reroll, back to RTBC as per #94
Comment #98
catchComing 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).
Comment #99
larowlanYes this could be done, do you think its in scope here or a follow-up?
Comment #100
larowlanHere 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
After
Comment #101
larowlanUpdated issue summary
Comment #101.0
larowlanUpdated issue summary.
Comment #103
larowlanFixes test fails.
Comment #104
larowlanOh 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.
Comment #105
jibrantbh I dono what is going on after #98. But I think we should add
options
module under dependencies inforum.info.yml
Comment #106
larowlanI did, but we need to make sure the field info type cache is primed before the default config is installed
Comment #107
larowlanfrom interdiff at #100
Comment #108
catchWow that was quick.
Interdiff looks great.
This should be handled by adding options as a dependency? I noticed a couple of minor comment issues but otherwise seems good to me.
Comment #109
larowlanI 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
Comment #110
larowlanTest passed, so yep, don't need the load.
Comment #111
larowlanGreen again, anyone for a review?
Comment #112
dawehnerThis method is already marked as deprecated
Let's try to skip out of scope changes.
Let's use '#title' for now, as part of the returning render array.
Afaik we do have interface now.
Is there no other way then directly querying the node_field_data/comment table? Can't we use EQ somehow?
Should we rather return the term interface?
Afaik we are using int not integer, see https://drupal.org/node/1354
nice!
Contains \....
Comment #113
larowlanFixes 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.
Comment #115
larowlanNode BC decorator is gone
Comment #116
larowlanBack to green
Comment #117
larowlanPostponed #2061997: Replace user_access() calls with $account->hasPermission() in forum module on this
Comment #117.0
larowlanUpdated issue summary.
Comment #118
larowlanAdded #2077435: Use a yml file to create the forum vocabulary
Removed the yml addition from this issue, scope creep
Comment #119
jibranJust a question, is $config still swapable? This should be "The forum config factory" I think.
Do we already have test for this in core?
@see #1978958-100: Convert comment_reply() to a Controller
We have
$this->translationManager
.Please update the comment. I am unable to understand it.
I think this should be 1 instead of TRUE.
80 char limit.
Is there any reason why we have not added all the ForumManger methods to interface?
Comment #120
larowlan1-7 thanks will fix
8, all public methods are, protected don't go on the interface, see comments above
Comment #121
larowlan1-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
Comment #122
jibran@catch was happy in #108. I am unable to find any other issue so back to RTBC.
Comment #123
larowlanPlease give the reviewers some commit credit here, its been a long process :)
Comment #123.0
larowlanUpdated issue summary.
Comment #123.1
larowlanUpdated issue summary.
Comment #124
alexpottNeeds reroll
Comment #125
larowlanreroll
Comment #127
Berdir#125: forum-page-route-1951268.reroll.patch queued for re-testing.
Comment #128
BerdirTestbot was broken, trying again.
Comment #130
larowlanComment #132
star-szrI could be wrong but it looks like some hunks were lost after #121, just diffed the patches.
Comment #133
larowlanFormat of field yml changed
Comment #135
larowlanTables 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.
Comment #137
larowlanmissing a use and wrong use of entity_load for the field
stupidest. reroll. ever.
Comment #139
larowlanThis time. please. really.
A new call to _forum_update_forum_index() was added in #1497374: Switch from Field-based storage to Entity-based storage
Comment #140
jibranO come on. July 1st huh. Sept. 1st is gone now :(
Comment #141
alexpottRe #123 and please give the reviewers some credit can you added suggested commit message please :)
Comment #142
larowlanPerhaps:
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.
Comment #143
jibranAnd back to RTBC.
Comment #143.0
jibranUpdated issue summary.
Comment #144
alexpottCommitted 88b6c93 and pushed to 8.x. Thanks!
Fixed the following in commit
Comment #145
alexpottOops forgot to add the tag... mind you this was RTBC July 1
Comment #146
ParisLiakos CreditAttribution: ParisLiakos commentedfollowup #2079477: ForumManager and ForumController use $this->translationManager->translate() instead of $this->t()
Comment #147
larowlanDraft change notice here https://drupal.org/node/2079767
Comment #148
andypostVery detailed change notice!
Comment #149
jibranYay!!! Thanks everyone great work :). Special thanks to @alexpott for approving the api change and committing it ;).
Comment #150
andypostSimple follow-up #2081429: Clean-up ForumManager doc-block
Comment #151
andypostAnd another follow-up #2081629: Optimize ForumManager
PS
lastVisit()
&unreadTopics()
fetches to much dataComment #152.0
(not verified) CreditAttribution: commentedAdd suggested commit message