Convert "Active forum topics" block to a View

I am here in Dublin and i will make this issues

I created a new view with two block displays:

New forum topics

• 5 Topics
• Sort by Post Date Desc

Active forum topics

• 5 Topics
• Sort by last comment date, Desc

The view selects following fields:

• Forum Nid (exlude from display)
• Forum Title

I selecting the Nid because i need to rewrite the link target as views doesnt provide any automatic option to to this.

The patch also remove the old block files

• ActiveForumBlock.php
• NewForumBlock.php

And remove the following function:

/**
 * Render API callback: Lists nodes based on the element's #query property.
 *
 * This function can be used as a #pre_render callback.
 *
 * @see \Drupal\forum\Plugin\block\block\NewTopicsBlock::build()
 * @see \Drupal\forum\Plugin\block\block\ActiveTopicsBlock::build()
 */
function forum_block_view_pre_render($elements) {
  $result = $elements['#query']->execute();
  if ($node_title_list = node_title_list($result)) {
    $elements['forum_list'] = $node_title_list;
    $elements['forum_more'] = array('#theme' => 'more_link', '#url' => 'forum', '#title' => t('Read the latest forum topics.'));
  }
  return $elements;
}

I changed the patch:

• It now uses the correct name of the block. It is "views_block:forum_topic_lists-block_2".
• dawehner did tell us not to change "block_2" and leave it as it is.
• I removed the test about "Display 2 items" because this setting is no longer inside of the block configuration but a global views setting. And this should be covered with a test elsewhere. So imho we don't need to check this anymore.
• "More" link is now uppercase as the test demanded.

here is the interdiff. :)

let's do a before and after screenshot too.

Thanks for working on the patch.

So it seems to be that if the items per page override is in, this functionality is not part of the active forum topic view, so it should not be tested on here. That is fine to remove for now.

Create new version of the patch:

• Fix the Problems in the last test and change in ForumNodeAccessTest the block name to the new one
• Change the Display Style to html list, because the old one was a html list

Here the New Version after the patch

New Version off add active topic block, add new topic blocks looks the same

New Version off the new Blocks

Here the old Versions before the patch:

Old add active topic block

old add new topic block

old blocks

We can now override the items per page, let's get this issue up to speed again.

Assigning to myself to see if I can resolve the test failure during the DrupalCon Prague Code Sprint.

Ok, after some troubleshooting, I found out that the tests fail simply because the views module is not included during test set up. However...

After adding the Views module as a required module for the test, the "Forum private node access" test fails on one of the next statements. This is caused by the fact that, since D8, when querying with the "node_access"-tag against a base table <> {node}, you need to explicitly define the base table using $query->addMetadata('base_table', 'forum_index') (See http://drpual.org/node/1885420 for a discussion)

Unfortunately, hook_views_data currently only allows you to specify the query tag, not the query metadata. I have opened a new issue for this #2099511: Change notice: Need support for adding metadata in hook_views_data()..

Attached is a patch that includes the patch for that issue to prove tests pass, but this issue is blocked by #2099511: Change notice: Need support for adding metadata in hook_views_data().. Setting to NR for the testbot.

Views yml file was missing in previous patch.

This was basically a straight rerole, so let's get it to RTBC

21: vdc-2020387.patch queued for re-testing.

Sorry, I tried really hard to get to this one tonight but I am completely out of steam. :( But please check the in-depth review at #2020399-54: Convert "Who's online" block to a View because some of that might apply here, too.

Will try and bang this out tomorrow.

Sorry, cancel that. #2020399-58: Convert "Who's online" block to a View Patch still applies though, so hopefully one of the other core maintainers can get to it.

Ok, dawehner and I subsequently hugged and made up, and now that #1957276: Let users set the block instance title for Views blocks in the Block UI is in, there's no reason not to go all out on block conversion patches anymore. YEAH.

I'd love for all of these block conversions to make it into the next alpha, so tagging.

This one is funny because in addition to removing a bunch of crappy custom code, it actually fixes a bug with the old block where it lies to you about the existence of more forum topics:

Committed and pushed to 8.x. Thanks!

Not sure what broke this, but it hadn't been retested since mid-November.

git revert d166be3

Reverted the earlier commit with 5aab968 and pushed to 8.x.

21: vdc-2020387.patch queued for re-testing.

Uh oh. :( Sorry, folks! :(

In fairness, who would've guessed anyone did anything with Forum module in 2 weeks? :P~

Attached is an updated patch which resolves the test failures. However, as indicated in #18, this patch contains a hunk from #2099511: Change notice: Need support for adding metadata in hook_views_data()., so is blocked until that lands.

One more reason for why HEAD was broken is most likely this:

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/query/Sql.php

+        foreach ($base_table_data['table']['base']['query metadata'] as $key => $value)
+        $query->addMetaData($key, $value);
+        $count_query->addMetaData($key, $value);

→ The foreach lacks a block; the $count_query line only adds meta data for the last $key and $value of the loop.

→ The foreach lacks a block; the $count_query line only adds meta data for the last $key and $value of the loop.

Yeah I had the same kind of trouble while writing tests in the other issue.

The patches to date delete the caching that this block implements. We should enable time based results caching for this block for a short duration. I'd say 5-10min. No need for output caching.

The issue this was postponed on has been committed.

New patch

• Removed the query metadata hunk implemented in #2099511: Change notice: Need support for adding metadata in hook_views_data()..
• Added time-based result caching (5 min), no output caching.

I try to figure out why we don't use time based output caching,
beside from that this looks perfect!

I suggested results based cache just to more closely mimic what we had before. I'd be fine with output cache as well.

+++ b/core/modules/forum/forum.module
@@ -555,23 +555,6 @@ function forum_form_node_form_alter(&$form, &$form_state, $form_id) {
-    $elements['forum_more'] = array('#theme' => 'more_link', '#url' => 'forum', '#title' => t('Read the latest forum topics.'));

The more link in the views output doesn't contain a title attribute - can that be added or is that something views doesn't support? If not, I guess we can live without it, but it would be a worthwhile addition for other views too.

Less custom code++
More customisability++

Shedding the 'too specific to be of any use, too rigid to customise' tag that has burdened forum since forever.

Screenshots of title issue
before


after

I suggested results based cache just to more closely mimic what we had before. I'd be fine with output cache as well.

Ah okay, I am fine with keeping that for now and think about more in a follow up.

The more link in the views output doesn't contain a title attribute - can that be added or is that something views doesn't support? If not, I guess we can live without it, but it would be a worthwhile addition for other views too.

Don't we kind of need it for accessibility as well?

We either have

<a href="/forum">More<span class="invisible"> forum topics</span></a>

Or the title.
If we can do the markup above with views, then we're fine I think.

As said on IRC, it feels like an actual usecase for a template.

Adds some markup to help screen-readers.
If this approach is acceptable, I consider this RTBC
Screenshot of markup/output

46: forum-block.46.patch queued for re-testing.

schema tests++

and the other display

This looks like it will be an improvement for accessibility! Great job.

+++ b/core/modules/forum/forum.module
@@ -624,6 +607,17 @@ function forum_theme_suggestions_forums(array $variables) {
+    $variables['more']['#link_text'] .= '<span class="visually-hidden"> ' . filter_xss_admin($view->getTitle()) .  '</span>';

I'd prefer if there was no markup in PHP. Though right now we don't have filter_xss_admin as a filter(easily added though). This to me looks like an opportunity to use twig blocks. http://twig.sensiolabs.org/doc/tags/block.html We haven't used them at all in core yet but this seems like a fit if we can get them to work. Duplicating views-view.html.twig is asking for trouble, that's why I suggest this. Also would need a suggestion hook potentially for the view id, if that doesn't already exist (thought d7 had these suggestions I don't know if they carried over).

@Cottser, I'll need you to help vet my crazy ideas:) Something to discuss at the weekly.

Fixed the failures.

+++ b/core/modules/forum/config/install/views.view.forum_topic_lists.yml
@@ -0,0 +1,235 @@
+  block_1:
...
+    id: block_1
...
+  block_2:
...
+    id: block_2

+++ b/core/modules/forum/lib/Drupal/forum/Tests/ForumBlockTest.php
@@ -116,7 +99,7 @@ public function testActiveForumTopicsBlock() {
+    $block = $this->drupalPlaceBlock('views_block:forum_topic_lists-block_1');

+++ b/core/modules/forum/lib/Drupal/forum/Tests/ForumNodeAccessTest.php
@@ -72,8 +72,8 @@ function testForumNodeAccess() {
+    $this->drupalPlaceBlock('views_block:forum_topic_lists-block_1');
+    $this->drupalPlaceBlock('views_block:forum_topic_lists-block_2');

If this goes green, can these blocks get names like block_active_topics, block_new_topics. Similar question was asked previously #5.

I'll gladdly roll it in if you don't mind @dawehner? Thanks for the fixes, nevertheless:)

If this goes green, can these blocks get names like block_active_topics, block_new_topics. Similar question was asked previously #5.

I won't do that because I hate abysmally to rename the generated machine names but people disagree with that.

@dawehner that's understandable... then would you be in favour if I opened up an issue to have the view's blocks create better generated machine names?

@dawehner that's understandable... then would you be in favour if I opened up an issue to have the view's blocks create better generated machine names?

Well, I am not sure whether it is worth to fight here, not sure whether there is anyone else agreeing here.

We can see if it has any interest, otherwise nice work getting this green:)
#2269711: Generate views' display ids by their display_title.

Looks and great again :)

I'm assuming the tests removed from #5 are still fine and the ranges from 2-20 changed to 5, 10, 20, 40 are still cool with everybody.

What's the deal with the more link underline?

put screenshots in the issue summary.

does this need profiling? or a follow-up for looking into that?

looking at the screenshots, seems a typo capitalizing Topics in "New forum Topics". fixed that.

adding profile contributor task doc to the issue summary
https://drupal.org/contributor-tasks/profiling

Re #69 seems that because the class of .more-link class is on the link itself and not the wrapping div of the link. Nice sport @larowlan

Also that may have some implications on this issue: #2031301: Replace theme_more_link() and replace with #type 'more_link'

I'm thinking that may need some CSS tweaks as I do prefer the one-less-div but I'll see if others agree?

@larowlan that's definitely something we missed when adding it here #890362: Links should not be indicated by color only

But well, this is not a problem which is related at all with the patch.

This needs a reroll, maybe mostly for PSR-4? https://groups.drupal.org/node/424758

Looking at where this stands.

Reroll with PSR-4. There is a remaining issue with the markup for the more link getting double escaped that I didn't quite have time to fix yet.

This patch introduces a template override for the more link and goes back to using the title attribute. Is this an acceptable way or is there a better one? It's pretty similar to what's being done with the system module's branding block.

Fixing the schema issue with slave being renamed to replica and adding the template that was missing from last patch.

Decided to drop the need for the additional template and moved the function along. But this is just a small detail.

What about the profiling?

+++ b/core/modules/views/views.module
@@ -16,6 +16,7 @@
+use Drupal\Core\Template\Attribute;

@@ -268,6 +269,13 @@ function views_theme_suggestions_node_alter(array &$suggestions, array $variable
+ * Implements MODULE_preprocess_HOOK() for views-more templates.
+ */
+function views_preprocess_views_more(&$variables) {
+  $variables['attributes'] = new Attribute();
+}
+
+/**

I don't think this is needed, the Attribute object is initialized in _theme() for attributes, content_attributes, title_attributes.

But maybe I'm missing something :)

Really like the use of addClass btw.

@Antti J. Salminen
Please feel free to fix the reviews. Just wanted to avoid explaining my idea and just implemented it.

@dawehner Thanks! I'll do that and try to take it forward from that.

Ok, this should be working now. At least profiling still remains.

so ....

This should be a very straightforward reroll with no real changes, hopefully tests pass again after it.

@Cottser
Well, profiling would be obviously a little bit unfair, but let's see what happens.

@dawehner just saying that because it's tagged. I'm fine either way :)

Yeah I guess we cannot not do it.

Here's an attempt at profiling the "Active forum topics block". This is with 50 topics containing 9 comments each for a total of 500 posts. Current is 8.0.x. Done as described in the contributor task. Only change to default config was to enable the APC Classloader. I suppose the "New forum topics" block also needs to be tested.

=== current..current compared (54233b929147b..54233d77d84ce):

ct  : 65,214|65,214|0|0.0%
wt  : 2,812,300|2,819,245|6,945|0.2%
cpu : 1,892,118|1,880,117|-12,001|-0.6%
mu  : 41,747,688|41,747,688|0|0.0%
pmu : 41,789,696|41,789,696|0|0.0%

=== current..forum-2020387-96 compared (54233b929147b..54233f7f69a7d):

ct  : 65,214|83,445|18,231|28.0%
wt  : 2,812,300|2,936,762|124,462|4.4%
cpu : 1,892,118|1,992,124|100,006|5.3%
mu  : 41,747,688|43,671,232|1,923,544|4.6%
pmu : 41,789,696|43,714,616|1,924,920|4.6%

---
ct = function calls, wt = wall time, cpu = cpu time used, mu = memory usage, pmu = peak memory usage

@Antti J. Salminen
Just to be sure, did you checked that the caches are warmed like finding all the views plugins (just curious).

@dawehner
I believe they should be. To me the profiling that was done on the recent comments block looks somewhat similar (https://www.drupal.org/node/1938062#comment-7576867). I think most of the extra time is spent in rendering the view.

I put the diff available online in the xhprof UI:
http://xhprof.facingworlds.com/?sort=ct&run1=54233b929147b&run2=54233f7f...

Rerolling because one of the files this deletes changed.

Seems to apply fine. You can definitely see the Active Forum Topics in the list of views.

There's a slightly different display of the block /admin/structure/block before & after:

This may not matter, but wanted to point it out. What manual testing should we do before this is marked RTBC? I really don't use forums much.

Have to address the missing views-more.html.twig #2036195: Remove views-more.html.twig and replace with #type link render arrays

Also have to look into why core/modules/forum/src/Plugin/Block/ForumBlockBase.php was rejected when applying the patch.

I think both of those files should be deleted.

This issue was marked as a beta target for the 8.0.x beta, but is not applicable as an 8.1.x beta target, so untagging.

This change is disruptive and so should be targeted for 8.2.x at this point.

reroll and re-export of the view.

Are these schema errors and unmet dependencies a problem with the view? Might need to be rebuilt or maybe go back to the last working one and run the Drupal updates.

Re-rolled.

Patch in #131 no longer applies.

Minor correction in schema.

Resolve many of the test failures.

I think I've taken this as far as I can. Does someone else want to take over? Otherwise I'm happy to accept advice/guidance.

I recommend rebuilding the view.

Patch created for 8.9.x-dev, Please review.

More test fixes.

Test Fixes.

More test fixes.

here is some failure test.

--

There was 1 failure:

1) Drupal\Tests\forum\Kernel\LegacyForumTest::testGetParents
Failed asserting that string matches format description.
--- Expected
+++ Actual
@@ @@
 @expectedDeprecation:
-%A  Drupal\forum\ForumManager::getParents() is deprecated in drupal:8.1.0 and is removed from drupal:9.0.0. Call loadAllParents() on taxonomy term storage directly. See https://www.drupal.org/node/3069599
+

ERRORS!
Tests: 37, Assertions: 123, Errors: 12, Failures: 1.

HTML output was generated

index fdd610aa26..e71591a58c 100644
--- a/core/modules/forum/tests/src/Kernel/LegacyForumTest.php
+++ b/core/modules/forum/tests/src/Kernel/LegacyForumTest.php
@@ -61,7 +61,7 @@ public function testGetParents() {
     ]);
     $subforum->save();

-    $legacy_parents = \Drupal::service('forum_manager')->getParents($subforum->id());
+    $legacy_parents = \Drupal::entityTypeManager()->getStorage('taxonomy_term')->loadAllParents($subforum->id());
     $parents = $this->termStorage->loadAllParents($subforum->id());
     $this->assertSame($parents, $legacy_parents);
   }

#155 Patch working fine also test case pass see screenshot.

Patch in #155 no longer applies d9, so created a new patch to support d9.

More test case fixes.

These changes make 'view' a dependency of the forum module.

Fixed More test, Please review.

vsujeetkumar++

thanks for debugging that -- changes look good.

Patch looks fine and now blocks are rendering through views.
Thanks for this contribution.

Patch #169 failed to apply , so re-rolling the patch. Kindly review a patch.

@hardik patel
I applie ur latest patch getting some hunks failed
refer screenshot

D10 version needed
At this time we need a D10.1.x patch or MR for this issue.