Problem/Motivation

The title says it.

This is part of #1823450: [Meta] Convert core listings to Views

From xjm's comment in #1823450-5: [Meta] Convert core listings to Views:
Would probably need forum data integration for {forum_index} to avoid potential performance regressions

Proposed resolution

Convert "Active forum topics" block to a View

Remaining tasks

User interface changes

Should be none.

Before (from #42)

After (from #68)

Note also from @webchick in #26:
this also "... actually fixes a bug with the old block where it lies to you about the existence of more forum topics"

Accessibility Improvements

  • ?

API changes

None?

Files: 
CommentFileSizeAuthor
#119 2020387-119.patch17.51 KBlokapujya
#115 2020387-115.patch16.88 KBlokapujya
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 93,937 pass(es), 26 fail(s), and 16 exception(s). View
#110 AfterActiveForumViewPatch.png56.62 KBmgifford
#110 BeforeActiveForumViewPatch.png52.51 KBmgifford
#109 forum-2020387-109.patch17.4 KBoadaeh
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch forum-2020387-109.patch. Unable to apply patch. See the log in the details link for more information. View
#103 forum-2020387-103.patch17.41 KBAntti J. Salminen
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,041 pass(es). View
#96 forum-2020387-96.patch17.37 KBAntti J. Salminen
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,597 pass(es). View
#92 forum-2020387-92.patch17.34 KBAntti J. Salminen
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch forum-2020387-92_0.patch. Unable to apply patch. See the log in the details link for more information. View
#92 interdiff-2020387-85-92.txt2.13 KBAntti J. Salminen
#85 forum-2020387-85.patch18.56 KBdawehner
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,141 pass(es), 75 fail(s), and 1 exception(s). View
#85 interdiff.txt2.91 KBdawehner
#84 forum-2020387-83.patch17.58 KBAntti J. Salminen
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,182 pass(es). View
#82 forum-2020387-82.patch16.87 KBAntti J. Salminen
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,141 pass(es), 77 fail(s), and 1 exception(s). View
#81 forum-2020387-81.patch16.29 KBAntti J. Salminen
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,177 pass(es), 2 fail(s), and 0 exception(s). View
#72 interdiff-2020387-63-72.txt550 bytesYesCT
#72 forum-2020387-72.patch16.49 KBYesCT
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,257 pass(es), 56 fail(s), and 19 exception(s). View
#69 Screenshot 2014-05-18 17.21.43.png26.28 KBlarowlan
#63 interdiff.txt2.34 KBdawehner
#63 forum-2020387-63.patch16.49 KBdawehner
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,605 pass(es). View
#58 forum-block-2020387-58.patch16.7 KBoadaeh
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,344 pass(es), 61 fail(s), and 0 exception(s). View
#52 forum-block.52.patch16.62 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] 64,227 pass(es), 22 fail(s), and 0 exception(s). View
#52 interdiff.txt2.58 KBlarowlan
#50 forum-block.50.patch16.62 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] 64,360 pass(es), 11 fail(s), and 0 exception(s). View
#50 interdiff.txt7.28 KBlarowlan
#46 Screenshot 2014-02-11 19.32.03.png36.09 KBlarowlan
#46 forum-block.46.patch16.44 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] 64,334 pass(es), 108 fail(s), and 0 exception(s). View
#46 interdiff.txt770 byteslarowlan
#42 Screenshot 2014-01-08 07.49.25.png15.14 KBlarowlan
#42 Screenshot 2014-01-08 07.49.18.png22.45 KBlarowlan
#39 2020387-39-views-forum-blocks.patch15.87 KBmr.baileys
PASSED: [[SimpleTest]]: [MySQL] 59,330 pass(es). View
#34 interdiff.txt650 bytesmr.baileys
#34 2020387-34-views-forum-blocks.patch16.63 KBmr.baileys
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2020387-34-views-forum-blocks.patch. Unable to apply patch. See the log in the details link for more information. View
#26 Screen Shot 2013-12-06 at 2.55.52 PM.png27.55 KBwebchick
#21 vdc-2020387.patch16.63 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 59,177 pass(es), 4 fail(s), and 0 exception(s). View
#20 2020387-20-views-active-forum-listing.patch14.97 KBmr.baileys
PASSED: [[SimpleTest]]: [MySQL] 58,887 pass(es). View
#18 2020387-18-views-active-forum-topics-listing.patch8.8 KBmr.baileys
FAILED: [[SimpleTest]]: [MySQL] 58,366 pass(es), 0 fail(s), and 6 exception(s). View
#15 views-forum-blocks-2020387-15.patch13.4 KBoadaeh
FAILED: [[SimpleTest]]: [MySQL] 58,870 pass(es), 0 fail(s), and 2 exception(s). View
#11 new_active_topic_block.png49.35 KBAndi-D
#11 new_blocks.png16.18 KBAndi-D
#11 old_blocks.png17.32 KBAndi-D
#11 old_add_active_topic_block.png46.98 KBAndi-D
#11 old_add_new_topic_block.png46.73 KBAndi-D
#10 drupal-issue-2020387_1.patch13.41 KBAndi-D
FAILED: [[SimpleTest]]: [MySQL] 56,527 pass(es), 0 fail(s), and 2 exception(s). View
#6 interdiff-2020387-3-5.txt3.74 KBYesCT
#5 drupal-issue-2020387.patch12.48 KBKars-T
FAILED: [[SimpleTest]]: [MySQL] 56,605 pass(es), 0 fail(s), and 2 exception(s). View
#3 drupal-issue-2020387.patch9.25 KBAndi-D
FAILED: [[SimpleTest]]: [MySQL] 58,433 pass(es), 0 fail(s), and 6 exception(s). View

Comments

oadaeh’s picture

Issue tags: +VDC

Forgot the tag.

Andi-D’s picture

Assigned: Unassigned » Andi-D

I am here in Dublin and i will make this issues

Andi-D’s picture

Assigned: Andi-D » Unassigned
Status: Active » Needs review
FileSize
9.25 KB
FAILED: [[SimpleTest]]: [MySQL] 58,433 pass(es), 0 fail(s), and 6 exception(s). View

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;
}

Status: Needs review » Needs work

The last submitted patch, drupal-issue-2020387.patch, failed testing.

Kars-T’s picture

Status: Needs work » Needs review
FileSize
12.48 KB
FAILED: [[SimpleTest]]: [MySQL] 56,605 pass(es), 0 fail(s), and 2 exception(s). View

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.
YesCT’s picture

FileSize
3.74 KB

here is the interdiff. :)

YesCT’s picture

Issue tags: +Needs screenshots

let's do a before and after screenshot too.

Status: Needs review » Needs work

The last submitted patch, drupal-issue-2020387.patch, failed testing.

dawehner’s picture

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.

Andi-D’s picture

FileSize
13.41 KB
FAILED: [[SimpleTest]]: [MySQL] 56,527 pass(es), 0 fail(s), and 2 exception(s). View

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
Andi-D’s picture

Here the New Version after the patch

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

new version add active topic block

New Version off the new Blocks

New Version off the new Blocks

Here the old Versions before the patch:

Old add active topic block

Old Version off add active topic block

old add new topic block

old version off add new topic

old blocks

old blocks

Andi-D’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, drupal-issue-2020387_1.patch, failed testing.

dawehner’s picture

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

oadaeh’s picture

Status: Needs work » Needs review
Issue tags: -Needs screenshots
FileSize
13.4 KB
FAILED: [[SimpleTest]]: [MySQL] 58,870 pass(es), 0 fail(s), and 2 exception(s). View

Updated patch.

I think the test is going to fail again, but I couldn't figure out where to look to fix it.

Status: Needs review » Needs work

The last submitted patch, views-forum-blocks-2020387-15.patch, failed testing.

mr.baileys’s picture

Assigned: Unassigned » mr.baileys

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

mr.baileys’s picture

Status: Needs work » Needs review
FileSize
8.8 KB
FAILED: [[SimpleTest]]: [MySQL] 58,366 pass(es), 0 fail(s), and 6 exception(s). View

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.

Status: Needs review » Needs work

The last submitted patch, 2020387-18-views-active-forum-topics-listing.patch, failed testing.

mr.baileys’s picture

Status: Needs work » Needs review
FileSize
14.97 KB
PASSED: [[SimpleTest]]: [MySQL] 58,887 pass(es). View

Views yml file was missing in previous patch.

dawehner’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
FileSize
16.63 KB
FAILED: [[SimpleTest]]: [MySQL] 59,177 pass(es), 4 fail(s), and 0 exception(s). View

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

Xano’s picture

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

webchick’s picture

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.

webchick’s picture

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.

mr.baileys’s picture

Assigned: mr.baileys » Unassigned
webchick’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +alpha target
FileSize
27.55 KB

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:

Forum block says there are more when there are not

Committed and pushed to 8.x. Thanks!

tim.plunkett’s picture

Title: Convert "Active forum topics" block to a View » HEAD BROKEN: Convert "Active forum topics" block to a View
Category: Task » Bug report
Priority: Normal » Critical
Status: Fixed » Reviewed & tested by the community

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

git revert d166be3

alexpott’s picture

Priority: Critical » Normal
Status: Reviewed & tested by the community » Needs work

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

alexpott’s picture

Title: HEAD BROKEN: Convert "Active forum topics" block to a View » Convert "Active forum topics" block to a View
Category: Bug report » Task
alexpott’s picture

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

The last submitted patch, 21: vdc-2020387.patch, failed testing.

webchick’s picture

Uh oh. :( Sorry, folks! :(

webchick’s picture

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

mr.baileys’s picture

Status: Needs work » Postponed
FileSize
16.63 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2020387-34-views-forum-blocks.patch. Unable to apply patch. See the log in the details link for more information. View
650 bytes

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.

sun’s picture

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.

dawehner’s picture

→ 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.

moshe weitzman’s picture

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.

ianthomas_uk’s picture

Status: Postponed » Needs work

The issue this was postponed on has been committed.

mr.baileys’s picture

Status: Needs work » Needs review
FileSize
15.87 KB
PASSED: [[SimpleTest]]: [MySQL] 59,330 pass(es). View

New patch

dawehner’s picture

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

moshe weitzman’s picture

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

larowlan’s picture

+++ 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
does my butt look big?

after
does my butt look big?

dawehner’s picture

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?

larowlan’s picture

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.

dawehner’s picture

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

larowlan’s picture

FileSize
770 bytes
16.44 KB
FAILED: [[SimpleTest]]: [MySQL] 64,334 pass(es), 108 fail(s), and 0 exception(s). View
36.09 KB

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

Status: Needs review » Needs work

The last submitted patch, 46: forum-block.46.patch, failed testing.

larowlan’s picture

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

The last submitted patch, 46: forum-block.46.patch, failed testing.

larowlan’s picture

Status: Needs work » Needs review
FileSize
7.28 KB
16.62 KB
FAILED: [[SimpleTest]]: [MySQL] 64,360 pass(es), 11 fail(s), and 0 exception(s). View

schema tests++

Status: Needs review » Needs work

The last submitted patch, 50: forum-block.50.patch, failed testing.

larowlan’s picture

Status: Needs work » Needs review
FileSize
2.58 KB
16.62 KB
FAILED: [[SimpleTest]]: [MySQL] 64,227 pass(es), 22 fail(s), and 0 exception(s). View

and the other display

Status: Needs review » Needs work

The last submitted patch, 52: forum-block.52.patch, failed testing.

mgifford’s picture

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

joelpittet’s picture

+++ 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).

joelpittet’s picture

Issue tags: +Twig

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

xjm’s picture

Issue tags: +beta target
oadaeh’s picture

Status: Needs work » Needs review
FileSize
16.7 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,344 pass(es), 61 fail(s), and 0 exception(s). View

Here's is a re-roll of @larowlan's patch, updated to the current state of core. I also removed the caching in the views, as that was causing my local tests to fail.

Status: Needs review » Needs work

The last submitted patch, 58: forum-block-2020387-58.patch, failed testing.

oadaeh’s picture

Status: Needs work » Needs review

58: forum-block-2020387-58.patch queued for re-testing.

The last submitted patch, 34: 2020387-34-views-forum-blocks.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 58: forum-block-2020387-58.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
16.49 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,605 pass(es). View
2.34 KB

Fixed the failures.

joelpittet’s picture

+++ 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:)

dawehner’s picture

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.

joelpittet’s picture

@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’s picture

@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.

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
93.97 KB

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.

larowlan’s picture

What's the deal with the more link underline?

YesCT’s picture

Issue summary: View changes

put screenshots in the issue summary.

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

YesCT’s picture

YesCT’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
16.49 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,257 pass(es), 56 fail(s), and 19 exception(s). View
550 bytes

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

dawehner’s picture

Issue tags: +Needs profiling
YesCT’s picture

Issue summary: View changes

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

joelpittet’s picture

Status: Needs review » Needs work

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?

mgifford’s picture

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

dawehner’s picture

Status: Needs work » Reviewed & tested by the community

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

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 72: forum-2020387-72.patch, failed testing.

Cottser’s picture

Issue tags: +Needs reroll

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

Antti J. Salminen’s picture

Assigned: Unassigned » Antti J. Salminen

Looking at where this stands.

Antti J. Salminen’s picture

Assigned: Antti J. Salminen » Unassigned
FileSize
16.29 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,177 pass(es), 2 fail(s), and 0 exception(s). View

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.

Antti J. Salminen’s picture

Status: Needs work » Needs review
FileSize
16.87 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,141 pass(es), 77 fail(s), and 1 exception(s). View

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.

Status: Needs review » Needs work

The last submitted patch, 82: forum-2020387-82.patch, failed testing.

Antti J. Salminen’s picture

Status: Needs work » Needs review
FileSize
17.58 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,182 pass(es). View

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

dawehner’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
2.91 KB
18.56 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,141 pass(es), 75 fail(s), and 1 exception(s). View

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

Cottser’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: -Needs reroll

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 :)

Cottser’s picture

Really like the use of addClass btw.

dawehner’s picture

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

Status: Needs review » Needs work

The last submitted patch, 85: forum-2020387-85.patch, failed testing.

The last submitted patch, 81: forum-2020387-81.patch, failed testing.

Antti J. Salminen’s picture

Assigned: Unassigned » Antti J. Salminen

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

Antti J. Salminen’s picture

Assigned: Antti J. Salminen » Unassigned
Status: Needs work » Needs review
FileSize
2.13 KB
17.34 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch forum-2020387-92_0.patch. Unable to apply patch. See the log in the details link for more information. View

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

dawehner queued 92: forum-2020387-92.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 92: forum-2020387-92.patch, failed testing.

dawehner’s picture

Issue tags: +Needs reroll

so ....

Antti J. Salminen’s picture

Status: Needs work » Needs review
FileSize
17.37 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,597 pass(es). View

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

dawehner’s picture

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

Cottser’s picture

Issue tags: -Needs reroll

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

dawehner’s picture

Yeah I guess we cannot not do it.

Antti J. Salminen’s picture

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
dawehner’s picture

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

Antti J. Salminen’s picture

@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...

Antti J. Salminen’s picture

FileSize
17.41 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,041 pass(es). View

Rerolling because one of the files this deletes changed.

dawehner queued 103: forum-2020387-103.patch for re-testing.

larowlan’s picture

Issue tags: +Needs reroll
oadaeh’s picture

Assigned: Unassigned » oadaeh
oadaeh’s picture

Assigned: oadaeh » Unassigned
FileSize
17.4 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,990 pass(es). View

A simple re-roll with only changes to account for Drupal updates.

oadaeh’s picture

Issue tags: -Needs reroll
oadaeh’s picture

FileSize
17.4 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch forum-2020387-109.patch. Unable to apply patch. See the log in the details link for more information. View

I found a bug in my previous patch.
This patch simply changes the content links from 'node/[nid]' to 'node/{{ nid }}'.

mgifford’s picture

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:
before the patch

after the patch

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.

jibran queued 109: forum-2020387-109.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 109: forum-2020387-109.patch, failed testing.

oadaeh’s picture

Issue tags: +Needs reroll
mgifford’s picture

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.

lokapujya’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
16.88 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 93,937 pass(es), 26 fail(s), and 16 exception(s). View

I think both of those files should be deleted.

Status: Needs review » Needs work

The last submitted patch, 115: 2020387-115.patch, failed testing.

xjm’s picture

Issue tags: -alpha target, -beta target

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.

lokapujya’s picture

Version: 8.0.x-dev » 8.2.x-dev
lokapujya’s picture

Status: Needs work » Needs review
FileSize
17.51 KB

reroll and re-export of the view.

Status: Needs review » Needs work

The last submitted patch, 119: 2020387-119.patch, failed testing.

lokapujya’s picture

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.

  • alexpott committed 5aab968 on 8.3.x
    Revert "Issue #2020387 by mr.baileys, Andi-D, dawehner, oadaeh, YesCT,...
  • webchick committed d166be3 on 8.3.x
    Issue #2020387 by mr.baileys, Andi-D, dawehner, oadaeh, YesCT, Kars-T:...

  • alexpott committed 5aab968 on 8.3.x
    Revert "Issue #2020387 by mr.baileys, Andi-D, dawehner, oadaeh, YesCT,...
  • webchick committed d166be3 on 8.3.x
    Issue #2020387 by mr.baileys, Andi-D, dawehner, oadaeh, YesCT, Kars-T:...

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

  • alexpott committed 5aab968 on 8.4.x
    Revert "Issue #2020387 by mr.baileys, Andi-D, dawehner, oadaeh, YesCT,...
  • webchick committed d166be3 on 8.4.x
    Issue #2020387 by mr.baileys, Andi-D, dawehner, oadaeh, YesCT, Kars-T:...

  • alexpott committed 5aab968 on 8.4.x
    Revert "Issue #2020387 by mr.baileys, Andi-D, dawehner, oadaeh, YesCT,...
  • webchick committed d166be3 on 8.4.x
    Issue #2020387 by mr.baileys, Andi-D, dawehner, oadaeh, YesCT, Kars-T:...

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.