Newsletter node: garbage output in view

That is the newsletter user view element that we always had, with two problems:

- The #theme functions that it is using no longer exists
- it is not defined as an extra field, so it can not be hidden.

The node shows it because they show the user with a special view mode.

This is also in todo.txt already.

This is the todo.txt row for this "simplenews_user_view: Uses old #theme functions"

So they should be '#type' => 'item' instead, and should be added with simplenews_entity_extra_field_info()?

A combination of things I guess, an item list for the list of subscriptions, a #type link for the link, maybe a fieldset around it, or something else. And that controlled by an extra field, yes.

Changed types to 'item' value and added a condition that allows fields to be hidden. Probably it needs some refactoring.

Good start.

1. +++ b/simplenews.module
   @@ -523,46 +523,48 @@ function simplenews_user_delete(UserInterface $account) {
   +        '#type' => 'user_profile_category',
   

   This also no longer exists. I'd suggest a fieldset, as commented above.

2. +++ b/simplenews.module
   @@ -523,46 +523,48 @@ function simplenews_user_delete(UserInterface $account) {
   +        $build['simplenews']['my_newsletters'] = array(
   +          '#type' => 'item',
   +          '#title' => '',
   +          '#markup' => t('Manage <a href="!url">subscriptions</a>', array('!url' => \Drupal::url('simplenews.newsletter_subscriptions_user', array('user' => $account->id())))),
   +        );
   

   It would be slightly different, but I would recommend to use #type link here, and make the whole text the link text.

We tried a fieldset but I think those can only be used in forms? We got an error saying something about #attributes. 'details' might be a better choice, but it looks kinda chunky.

I changed the type from the 'user_profile_category' to 'details' because 'fieldset' gives an error, as Arlid said. Changed the #type for 'Manage subscriptions' to 'link' and made the whole text linkable.

I guess we want some kind of test here, checking that the output is correct. May be necessary to use xpath to identify the correct elements.

Instead of pushing the whole chunk into an if-body, we can set $build['simplenews']['#access'] to the value of (bool) $display->getComponent('simplenews'). That will at least make the diff a lot nicer :)

By the way, neither me nor @mbovan could reproduce this on the node view, only on the user view. Something changed there since original issue summary?

It might make the patch bigger, but an if (getComponent()) is the common and better pattern. with #access, you will waste processing power loading those subscriptions and then not display them.

Access is often easier/better, but not here.

Can you post screenshots of how it looks, with/without details?

(or show me)

Here are the screenshots with and without details type.

Subscriptions are now rendered as a list.
Here are two screenshots how changes look like with subscriptions and without subscriptions.

Committed and fixed.

That's not a critical problem ;) And it might be something that we can't fix in simplenews.

What you're seeing is the user, viewed with the compact user mode. You can reconfigure it and it will go away. (workaround => not critical, if we apply the same rules as core ;)).

The problem is that it apparently gets added there by default, because extra fields are made visible there by default it seems. Don't know why. We can only set a default visiblity, so the best we can do I guess is to hide it always by default.

Now it's shown in the user and not in the node.
User:

Node:

1. +++ b/simplenews.install
   @@ -135,6 +135,10 @@ function simplenews_install() {
   +  entity_get_display('user', 'user', 'default')
   

   entity_get_display() is deprecated and will be removed before Drupal 9. We can use \Drupal::entityManager()->getStorage('entity_view_display').

   Btw, should we care about the case when there is no user view display? With this patch if user view display is not present we will get the same thing as without the patch.

2. +++ b/simplenews.module
   @@ -14,6 +14,7 @@ use Drupal\Core\Render\Element;
   +use Drupal\node\Entity\NodeType;
   

   Not needed.

I made those changes.

+++ b/simplenews.install
@@ -135,6 +135,10 @@ function simplenews_install() {
+  \Drupal::entityManager()->getStorage('entity_view_display')->load('user.user.default')
+    ->setComponent('simplenews', array(
+      'label' => 'hidden',
+      'type' => 'simplenews',

This wasn't trivial to figure out, lets add a comment about why we do this. That we hide this by default but want to show it for the default view mode.

Also, can we write a test? should be possible to assert that this is not shown on a node in one of the existing tests? We might need to enable the show author setting of the node type for it to work, though.

Test done and comments added.

looks good to me

+++ b/src/Tests/SimplenewsTestUI.php
@@ -0,0 +1,40 @@
+ * Test the UI aspects of simplenews.
+ *
+ * @group simplenews
+ */
+class SimplenewsTestUI extends SimplenewsTestBase {
+
+  /**
+   * Test the visibility of subscription user component.
+   */
+  public function testSubscriptionUserComponentVisiblity() {
+    // Create article content type.

We already have plenty of UI tests, it is a bit weird to add a UI test class now.

Also, tests always have a Test suffix.

Instead, I think that this would fit well in SimplenewsAdministrationTest::testContentTypes(), where we already create a node and visit it.

Needs to be debugged by Berdir.
I moved the test there and now fails like there where nothing changed.

See #2551959: Hardcoded usage of compact view mode in template_preprocess_node() and template_preprocess_comment() is problematic.

Considering that, let's just disable it by default and users have to enable it when they want to use it.

Fixing the tests.

Committed.