Remove forum_menu_local_tasks_alter() hack and instead add links in ForumController::build

Thanks @bjlewis2!

So probably these messages should simply not be displayed inside the actions link markup. We can't backport anything here because it would be a UI change, but the only real oddities are in D8 in any case.

This might also have been affected by #1167350: Action links were ignored .

I've attached a patch to #1167350: Action links were ignored , it will fix this problem

Unpostponing, since the markup being output here seems to be improper usage of action links.

We probably need to move those "messages"/notices into the page markup.

Action links broken the UI pattern because it's called "Action link" only. I still think display the text is needed in this case.

Patch seems need more better access handle.

I applied the patch in #6, created the Test user and tested as anonymous as well as logged in as the Test user and the results were the same as indicated in #7.

Note: Applying this patch causes the patch in http://drupal.org/node/953214#comment-6693922 to not apply.

The result of the discussion in #1167350: Action links were ignored was that Forum module should not use hook_menu_local_tasks_alter() to expose/show these links in the first place, because these are not action links.

So what we want to do here is to move that code to an entirely different spot, but essentially try to retain the approximate position.

The most simple approach would be to move the code into an own helper function (i.e., renaming the current hook implementation function), make it accept a $build render array, and make it add a container that contains the respective link or markup, depending on the configuration settings, and apply a very low negative weight to the container. (i.e., essentially doing what the current action links are doing, just without action links.)

function forum_login_box(&$build) {
  $build['forum_login'] = array(
    '#type' => 'container',
    '#weight' => -100,
  );
  $build['forum_login']['link'] = array(
    '#type' => 'link',
    '#title' => t('Log in or register to post new content in the forum'),
    '#href' => 'user',
    '#options' => drupal_get_destination(),
  );
}

Then, inject calls to this helper method from every forum page callback that needs it.

That would be the most simple way. However, I want to amend that this messaging is not really part of the actual page callback content (which is probably more or less the reason for why it has been moved into hook_menu_local_tasks_alter()).

Therefore, an alternative approach would be to turn this container into a real and new block, which just outputs the necessary link/content, and make forum_install() enable and position that block into the 'content' region with a low weight. As a result, site builders and themers will be able to move this messaging block very easily to a different place/region (e.g., into a sidebar).

Overall, the block approach would make more sense to me.

Not sure I like the block approach, not sure why though.
I think it has to do with making block a dependency for forum, but I can't put my finger on it.

Well, my longer term goal would be a header area in a view, but we need some other things resolved before we can do that eg #1867642: Expose forum tables to views.
But I am struggling to come up with something that doesn't feel wrong, after re-reading sun's comments I guess block is the way to go, given most other content is moving to a block (eg site name etc).

@stevepurkiss: Forum containers + forums are taxonomy vocabularies, forum posts are nodes. Both vocabularies and nodes are entities in D8, so that's done already. However, that's off-topic here. :)

#6: forum-move-access-error-into-template-1853072-6.patch queued for re-testing.

#18: forum_access_messages-1853072-18.patch queued for re-testing.

Yup, this patch fails to apply in many, many ways right now. It will have to be re-built if the problem still exists.

Added images inline

Pretty much everything seems to have changed since the last roll a year ago in #18.

This blocks #2347465: Convert all instances of #type link/links to convert to use routes - over there they're just going to remove the hook and uncomment the tests - so we need to reinstate that functionality here.
Bumping accordingly

Logged in as the test user looking at the forum without ability to post it as per the description in the summary:

I think this is ready to go. Got it up and running here for the next 5 hrs http://sb1cc35baa618c1d.s3.simplytest.me/

The code looks good to me.

Can someone explain why we cannot solve the bug as part of forum_menu_local_tasks_alter()?

+++ b/core/modules/forum/src/Controller/ForumController.php
@@ -49,21 +74,39 @@ class ForumController extends ControllerBase {
+      $entity_manager->getAccessControlHandler('node'),
+      $entity_manager->getFieldMap(),
+      $entity_manager->getStorage('node_type')

Please try to not do this as part of the constructor?

35 because they're abusing it to show text in the header in the case if the unauthorized user, ie local action with no link.
36 that is in the factory

36 that is in the factory

Well, this is not about "the constructor" but about the construction of objects. Executing anything there opens potentially a lot of race conditions, circular dependencies, slow code paths etc.

Can you elaborate what you mean? This constructor signature makes it explicit what the dependencies are. The alternative is to inject the whole entity manager but that obscures the real dependencies and would mean we'd have to mock a load of methods on the entity manager if we ever wanted to add unit tests.

Fwiw I've had several patches elsewhere that inject the entity manager knocked back in favor of this level of explicitness so assume there is no policy, instead its more of a reviewer preference thing?

It's okay here, not okay in services.

Here is rerolled patch from #32, as it doesn't apply anymore.

Triggering testbot.

re-roll

+++ b/core/lib/Drupal/Core/Utility/Token.php
@@ -37,7 +37,7 @@
- * $node = Node::load(1);
+ * $node = node_load(1);

Base rebase?

re-rolled without that hunk

Looks good to me. I went through and tested this patch in SimplyTest.me.

Screenshot still looks like #34.

This looks like a testable bug.

Grep for 1853072 in HEAD, you'll see that there IS test coverage, we just removed it temporarily.
This issue should restore that.

thanks @timplunkett - restored test

Can we get a followup to add a test for the anonymous case - i.e. to check that the anonymous user has a link to login.

This issue addresses a major bug and is allowed per https://www.drupal.org/core/beta-changes. Committed bfe4403 and pushed to 8.0.x. Thanks!

Follow up is #2379459: Add a test for forum action links for anon users