Problem/Motivation

Some base node fields (Title, Promote and Sticky), are missing descriptions. This causes an error message in views - specifically Views UI on the add field(s) page.

Proposed resolution

Chosen solution:
Leave the description column blank when the entity type does not specify it:

Remaining tasks

CommentFileSizeAuthor
#79 interdiff.txt1.54 KBdawehner
#79 2509722-79.patch4.79 KBdawehner
#76 interdiff.txt2.31 KBdawehner
#76 2509722-76.patch4.75 KBdawehner
#71 2509722-70.patch4.99 KBdawehner
#71 interdiff.txt770 bytesdawehner
#69 2509722-69.patch4.98 KBdawehner
#69 interdiff.txt4.87 KBdawehner
#65 interdiff-2509722-59-65.txt2.88 KBLendude
#65 error_missing_help_-2509722-65.patch3.66 KBLendude
#65 error_missing_help_-2509722-65-TEST_ONLY.patch1.99 KBLendude
#62 error_missing_help-2509722-62.patch1.77 KBLendude
#62 interdiff-2509722-59-62.txt1.79 KBLendude
#60 error_missing_help-2509722-60.patch944 bytesLendude
#59 interdiff-2509722-36-59.txt1.79 KBrakesh.gectcr
#59 error_missing_help_-2509722-59.patch1.72 KBrakesh.gectcr
#38 Screen Shot 2015-11-13 at 14.48.55.png28.02 KBNickDickinsonWilde
#38 Screen Shot 2015-11-13 at 14.41.15.png28.15 KBNickDickinsonWilde
#38 Screen Shot 2015-11-13 at 14.39.50.png31.65 KBNickDickinsonWilde
#36 error_missing_help_-2509722-36.patch1.88 KBNickDickinsonWilde
#30 error_missing_help_-2509722-30.patch1013 bytesNickDickinsonWilde
#17 2509722-17.patch1.33 KBNitesh Pawar
#14 after.png44.43 KBsnehi
#14 before.png42.61 KBsnehi
#10 Patching_error_report.txt149 bytessnehi
#9 some_node_fields-2509722-9.patch1.37 KBNickDickinsonWilde
#3 some_node_fields-2509722-3.patch1.36 KBNickDickinsonWilde
#1 some_node_fields-2509722-1.patch634 bytesNickDickinsonWilde
Screenshot 2015-06-21 15.36.57.png19.28 KBNickDickinsonWilde
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

NickDickinsonWilde’s picture

NickDickinsonWilde’s picture

Issue summary: View changes
NickDickinsonWilde’s picture

FileSize
1.36 KB

oops only uploaded 1/3 of the patch! full patch now.

NickDickinsonWilde’s picture

tagging for attention ;)

snehi’s picture

Status: Needs review » Needs work

Hi @Nick i am having query on vocab used in

A boolean indicating whether the node is stuck at the top of lists

Can we change the word

stuck

Otherwise patch is looking good.

NickDickinsonWilde’s picture

Status: Needs work » Needs review

@snehi, Do you think

A boolean indicating whether the node is should sort first at the top of the default homepage view and others.

or

A boolean indicating the node should sort before non sticky nodes.

would be better, or got any suggestions?
thanks
Nick

snehi’s picture

Status: Needs review » Needs work

I think

A boolean indicating the node should stays at top of content lists
NickDickinsonWilde’s picture

Status: Needs work » Needs review
FileSize
1.37 KB

Slight further change; conforms to the style/standard of the other descriptions better:

A boolean indicating whether the node should sort to the top of content lists.

Sound good to you?

snehi’s picture

Status: Needs review » Needs work
FileSize
149 bytes

Why Description is on second line might we due to which it is giving error.

Nitesh Pawar’s picture

Status: Needs work » Needs review

I applyed the same patch on my d8 git repo.
" Checking patch core/modules/node/src/Entity/Node.php...
Applied patch core/modules/node/src/Entity/Node.php cleanly."

NickDickinsonWilde’s picture

It applied fine on my system and the test system.
@Snehi: that was a full patch, if it was onto something already pasted with the older patch it wouldn't work, was it a clean (new) checkout?

snehi’s picture

Status: Needs review » Reviewed & tested by the community

My mistake. This is RTBC now.

snehi’s picture

FileSize
42.61 KB
44.43 KB

Adding Screenshot for the reviews.

Before

before_135.png

After

after_140.png

tstoeckler’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/node/src/Entity/Node.php
@@ -429,6 +430,9 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
+      ->setDescription(t(
+        'A boolean indicating whether the node is visible on the front page.'
+        ))

@@ -443,6 +447,9 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
+      ->setDescription(t(
+        'A boolean indicating whether the node should sort to the top of content lists.'
+        ))

This formatting is very non-standard in core. Can we just collapse that to one line please?

NickDickinsonWilde’s picture

Those lines were going over the 80 char - and as per coding standards

In general, all lines of code should not be longer than 80 characters.
Lines containing longer function names, function/class definitions, variable declarations, etc are allowed to exceed 80 characters.

I figured those should be split to decrease that, is that not the case?

Nitesh Pawar’s picture

Status: Needs work » Needs review
FileSize
1.33 KB

collapsed description to one line

ameymudras’s picture

Status: Needs review » Reviewed & tested by the community

Its working good now. Looks like RTBC to me

tstoeckler’s picture

Re #16: This fits into the

Lines containing longer function names, function/class definitions, variable declarations

(emphasis mine) part I think. Maybe you can amend the documentation with this example, so that it is more clear?

In any case, looks good now.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 17: 2509722-17.patch, failed testing.

NickDickinsonWilde’s picture

Status: Needs work » Needs review

CI error probably...

charginghawk’s picture

#17 fixed this issue for me - Title had "Error: missing help", but no more.

NickDickinsonWilde’s picture

Issue tags: -Needs subsystem maintainer review +rc target triage Needs subsystem maintainer review
NickDickinsonWilde’s picture

Issue tags: -rc target triage Needs subsystem maintainer review +rc target triage, +Needs subsystem maintainer review
Sagar Ramgade’s picture

Status: Needs review » Reviewed & tested by the community

Looks like RTBC to me.

xjm’s picture

Title: Some Node Fields missing a description » "Error: missing help" in Views for Node fields without descriptions
Component: node system » views.module
Status: Reviewed & tested by the community » Needs review
Issue tags: -rc target triage +Needs usability review, +VDC

So, I actually don't think this is the correct fix. Having a description for a field is not required, and in fact, having redundant text in the UI is actually a UX issue. We've done a lot of work to remove redundant descriptions in D8, and this will add some back to the node edit form itself, e.g.:

Title
[ ]
The node title.

Tagging for usability review to confirm my assessment.

I think the correct fix would either be to change how Views responds to a missing description, or to add some supplementary information in Views on a per-field basis. Depending on the nature of the fix, it could probably go into a patch release safely.

xjm’s picture

Status: Needs review » Needs work
Issue tags: -Needs usability review

Actually I'm just going to go ahead and NW this; I know that this would be a usability regression without needing @Bojhan to confirm it for me. ;)

xjm’s picture

@NickWilde asked what the correct step forward in this issue would be. I think it will look something like this:

diff --git a/core/modules/views/src/EntityViewsData.php b/core/modules/views/src/EntityViewsData.php
index 2069271..14bafa0 100644
--- a/core/modules/views/src/EntityViewsData.php
+++ b/core/modules/views/src/EntityViewsData.php
@@ -347,6 +347,9 @@ protected function mapSingleFieldViewsData($table, $field_name, $field_type, $co
     if ($description = $field_definition->getDescription()) {
       $views_field['help'] = $description;
     }
+    else {
+      // @todo Provide a fallback here.
+    }
 
     // Set up the field, sort, argument, and filters, based on
     // the column and/or field data type.
xjm’s picture

Issue tags: +Needs tests

Also, it will need tests for whatever fallback behavior we do implement. :)

NickDickinsonWilde’s picture

Well looked into it a bit. I found there is a sort-of fallback method already. Patch attached using that.
Benefits I see: already available and effective
Cons: per field fix rather than an actual fallback methodology.
(admittedly back a bit to the original thought rather than xjm's suggestion and I'm totally alright with looking into making a *real* fallback method if it is really needed - but if so where would I source the data from.)

NickDickinsonWilde’s picture

Status: Needs work » Needs review
xjm’s picture

That seems like a nice non-disruptive fix for the Node module cases. The downside however is that any other entity types that also don't need to provide a description for certain base fields would still have the unexpected "Error: missing help" message.

Maybe we could combine #28 and #30? For the automatically generated entity type data, fall back to an empty string if there is not a description, but then modules like Node can optionally alter in their help for these fields if desired?

NickDickinsonWilde’s picture

Should #28 and #30 be combined in one patch or make a separate issue for one of them ? On my own projects, I'd normally commit patches like that separately.

Lendude’s picture

Number of other issues about this, with some more discussion about what the right fix is for this.

xjm’s picture

Nice finds @Lendude. We could probably mark two as duplicates of a third.

@NickWilde, I'd do both changes in a single patch here, since they are both aspects of the same underlying issue with the automatic entity views data.

NickDickinsonWilde’s picture

@xjm alright, new patch attached.
Given the change, I don't think it needs a test - it just is using the label which is already required. More comment than code really.

Lendude’s picture

Oh and another duplicate one!

@xjm yeah I agree, I'm not really up to speed enough on this to decide what the best issue is to follow up as I'm still unclear about what the proper fix for this is. I feel the route in #2424065: Remove "Error: missing help" messages is better that what we have here because

+++ b/core/modules/views/src/EntityViewsData.php
@@ -347,6 +347,13 @@ protected function mapSingleFieldViewsData($table, $field_name, $field_type, $co
+      $views_field['help'] = $views_field['title'];

would just show me the same thing twice. I'd rather not see anything if it doesn't really add any extra depth (but I'm no UX expert by any means).

Some updated screenshots with the patch applied might also be helpful in deciding what path to take.

NickDickinsonWilde’s picture

Node specific fix:

Example of the look of the blank help version of the generic fix:

Example of the look of the title as help version of the generic fix: (makes me think if this is the chosen one it should append a period to the end).

Before: (obviously unacceptable)

rakesh.gectcr’s picture

xjm’s picture

OK, now I think we're ready for a usability review, since we are only changing the Views UI. :)

tstoeckler’s picture

I disagree with the recent changed. In theory @xjm you are 100% spot on about the useless descriptions but the fact of the matter is that throughout all of our content entities basically all fields have these useless descriptions. If we want to change that we should do so wholesale. I think we should still fix views but IMO that's a different issue.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

First, I totally agree with the problem that these descriptions in general are not helpful, but
I also agree 100% with @tstoeckler: These descriptions are coming from our autogenerated descriptions and the fact that the entity base field titles/descriptions aren't helpful,
is not the fault of this issue. Fixing this is classical scope creep.

Let's deal with that problem in #2615558: Improve base field titles / descriptions

RTBCing #17 again

xjm’s picture

Status: Reviewed & tested by the community » Needs work

Thanks @dawehner! We still need both tests and a usability review, though.

Edit: I did not understand that @dawehner meant he was RTBCing #17. That patch is not acceptable; see my next comment for why.

dawehner’s picture

Fair point, but yeah its also a general question whether we should mention missing help like that or just provide an empty message, as this could be enough ...

xjm’s picture

BTW:

I disagree with the recent changed. In theory @xjm you are 100% spot on about the useless descriptions but the fact of the matter is that throughout all of our content entities basically all fields have these useless descriptions. If we want to change that we should do so wholesale. I think we should still fix views but IMO that's a different issue.

The reason I rejected adding the descriptions as the earlier patch did had nothing to do with Views. The previous patch was changing the node entity type itself by adding new descriptions and thereby changing the node edit form. That's not an allowed change in 8.0.x, and a usability regression, plus impacts the product and yada yada because it's the most important form in Drupal. Whereas "Error: missing help" for a totally legit entity type obviously is not the correct behavior, so we should fix that in a way that only impacts Views.

tstoeckler’s picture

Hmm... fair point about the node edit form, I did not consider that. So it seems like there's no good solution to the issue here without increasing the scope :-/

dawehner’s picture

So I'm wondering whether we should get rid of descriptions/titles as concept on the longrun on the main level on base fields but rather think about
adding specific titles/descriptions onto the form and maybe view level and introduce a new hook/registration mechanism for those kind of non storage related metadata.

NickDickinsonWilde’s picture

Status: Needs work » Needs review

So I'm thinking that degree of discussion/scope creep required to fix the underlying issue is too much - enough to push it at LEAST to 8.1. However I think we all agree that there shouldn't be errors immediately visible especially in moderate level tasks like Views UI.

Therefore, I think the best thing to do right now is to use the patch from #30 which fixes the issue for the node fields in ViewsUI with no other impacts (ie doesn't affect the node add/edit forms). After 8 is out a full discussion between people with more knowledge/clout than me is probably in order but right now lets just get the error message gone.

xjm’s picture

Status: Needs review » Needs work

This is totally acceptable for 8.0.x and I bet we could even get it in for 8.0.1. #30 is non-disruptive and only 1K, and the many duplicates show that this is an annoying bug that will affect many sites. If we decide not to alter in the supplemental descriptions for Node based on the usability review, it's even smaller.

It still needs test coverage though, so setting back to NW for that. Thanks!

xjm’s picture

Issue summary: View changes

Updating the summary to clarify the remaining tasks.

xjm’s picture

Issue summary: View changes

Oops, fixing which screenshots are where.

NickDickinsonWilde’s picture

Since #30 isn't fixing the underlying issue so should the test just be confirming that 'error: missing help' is not appearing for any core fields in views UI? Or is there anything else it should be testing?

xjm’s picture

Priority: Normal » Major

@NickWilde, yep, that's part of it. Quoting my notes from the summary:

The patch needs automated tests for the fallback behavior (no "Error: missing help" for content entities, but still an "Error: missing help" for a non-entity data provider).

Turns out this bug also confused and misled contributors who were working on #2458223: Duplicated field handlers in field UI for some base table fields. Given the number of duplicates and the fact that this unsightly but otherwise harmless bug is actually causing a lot of people to think Views is broken, promoting to major.

Bojhan’s picture

If the information shown has little to no value, it is better left empty than either 1) duplication or 2) indication that its missing. Neither are really helpful to the user, and time is better spend scanning the list of options - than reading a description that has little propositional value.

xjm’s picture

Issue tags: -Needs usability review

Okay, thanks @Bojhan!

So based on @Bojhan's recommendation, let's remove node_views_data_alter() entirely remove the redundant entries in node_views_data_alter(), and just display a blank table cell for fields which do not have a description on the entity type.

Edit: changing my recommendation after reviewing the patch again; will post a followup comment.

xjm’s picture

  1. +++ b/core/modules/node/node.views.inc
    @@ -15,3 +15,15 @@ function node_views_wizard() {
    +  $data['node_field_data']['title']['help'] = t('The node title.');
    +  $data['node_field_revision']['title']['help'] = t('The node title.');
    

    Let's remove these two lines since they are mostly redundant.

  2. +++ b/core/modules/views/src/EntityViewsData.php
    @@ -347,6 +347,13 @@ protected function mapSingleFieldViewsData($table, $field_name, $field_type, $co
    +      $views_field['help'] = $views_field['title'];
    

    Instead of this, fall back to an empty string.

rakesh.gectcr’s picture

Assigned: Unassigned » rakesh.gectcr
rakesh.gectcr’s picture

Assigned: rakesh.gectcr » Unassigned
Status: Needs work » Needs review
FileSize
1.72 KB
1.79 KB

@xjm

I have done the changes you mentioned.

Lendude’s picture

Hmm I feel the place to fix this is were the error message gets set in the first place. This is pretty much a reroll from #2424065: Remove "Error: missing help" messages.

Either way this still needs tests.

snehi’s picture

@Lendude
Can you please provide the interdiff ?

Lendude’s picture

#59 was mostly to suggest an alternative spot to fix this. Rolled #59 en #60 together so an interdiff would actually make sense. Interdiff is between #59 and #62

dawehner’s picture

+++ b/core/modules/views/src/ViewsDataHelper.php
@@ -115,7 +115,11 @@ public function fetchFields($base, $type, $grouping = FALSE, $sub_type = NULL) {
+                  // If no help is provided default to an empty string.
+                  if ($string == 'help') {
+                    $strings[$field][$key][$string] = '';
+                  }
+                  elseif ($string != 'base') {
                     $strings[$field][$key][$string] = SafeMarkup::format("Error: missing @component", array('@component' => $string));

I'm still not sure whether we should keep having that being explicit there, so people are actually aware of it, wile working on views integrations ...

xjm’s picture

Yeah, agreed @dawehner, that is why I recommended the fallback in EntityViewsData instead. We should not display an error for auto-generated fields from the entity data. But there is still a case for developers writing integration for other tables to see this message. So I'd prefer to do #59 instead to fix the bug, and discuss changing the behavior of Views overall in a separate issue since that is more disruptive and a separate scope.

Lendude’s picture

Sounds like we have picked a solution then. Updated the summary to reflect the choses solution.

Updated the patch in #59 and added some tests per @xjm: no "Error: missing help" for content entities, but still an "Error: missing help" for a non-entity data provider.

Lets see if these pass.

The last submitted patch, 65: error_missing_help_-2509722-65-TEST_ONLY.patch, failed testing.

Lendude’s picture

Issue tags: -Needs tests

So this now has tests

dawehner’s picture

+++ b/core/modules/views/src/EntityViewsData.php
@@ -375,6 +375,13 @@ protected function mapSingleFieldViewsData($table, $field_name, $field_type, $co
+      // No description set on the field definition. If a better description
+      // should be used just for Views UI you can use hook_views_data_alter() in
+      // a module.views.inc file.
+      // @see node_views_data_alter().
+      $views_field['help'] = " ";

I'm a bit confused whether we should fix this just for entities or simple make help an empty string by default more down the road?

dawehner’s picture

So yeah let's drop the requirement to have a help text. It seems to cause more harm than good.

Status: Needs review » Needs work

The last submitted patch, 69: 2509722-69.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
770 bytes
4.99 KB

One tiny detail ...

dawehner’s picture

Version: 8.0.x-dev » 8.1.x-dev
dawehner’s picture

Version: 8.1.x-dev » 8.0.x-dev
Lendude’s picture

Couple of nitpicks:

  1. +++ b/core/modules/views/src/ViewsDataHelper.php
    @@ -114,10 +114,14 @@ public function fetchFields($base, $type, $grouping = FALSE, $sub_type = NULL) {
    +                // No description set on the field definition. If a better description
    +                // should be used just for Views UI you can use hook_views_data_alter() in
    +                // a module.views.inc file or implement a custom entity
    

    Truncate at 80 characters.

  2. +++ b/core/modules/views_ui/src/Tests/HandlerTest.php
    @@ -247,6 +255,24 @@ public function testNoDuplicateFields() {
    +   * Ensures that the Error: missing help message is not shown for entity
    +   * field handlers but is shown for other field handlers,
    

    This comment is no longer right

Also $strings[$field][$key]['help'] is now not set, it should be set to something (in previous version of the patch it was set to a space, which is what the test currently tests for)

Status: Needs review » Needs work

The last submitted patch, 71: 2509722-70.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
4.75 KB
2.31 KB

Thank you @Lendude for the quick feedback

Status: Needs review » Needs work

The last submitted patch, 76: 2509722-76.patch, failed testing.

Lendude’s picture

  1. +++ b/core/modules/views/src/ViewsDataHelper.php
    @@ -114,6 +114,15 @@ public function fetchFields($base, $type, $grouping = FALSE, $sub_type = NULL) {
    +                  $strings[$field][$key][$string] = '';
    
    +++ b/core/modules/views_ui/src/Tests/HandlerTest.php
    @@ -247,6 +255,23 @@ public function testNoDuplicateFields() {
    +    $this->assertTrue(count($this->xpath('//td[contains(@class, description) and text()=" "]')), 'Empty description found');
    

    value and test value don't match

  2. +++ b/core/modules/views/src/ViewsDataHelper.php
    @@ -114,6 +114,15 @@ public function fetchFields($base, $type, $grouping = FALSE, $sub_type = NULL) {
                       if ($string != 'base' && $string != 'base') {
    

    do we also want to remove this double check on the same string while we are at it?

dawehner’s picture

Status: Needs work » Needs review
FileSize
4.79 KB
1.54 KB

do we also want to remove this double check on the same string while we are at it?

Yeah, I guess its right to drop this little gem.

Sure here it is

Lendude’s picture

Status: Needs review » Reviewed & tested by the community

@dawehner nice! Looks good to me now.

Manually tested #79 and looks good there too.

damiankloip’s picture

+1

catch’s picture

Status: Reviewed & tested by the community » Fixed

This looks like a good solution to not cluttering the entity forms but keeping descriptions in Views where they're useful.

  1. +++ b/core/modules/node/src/NodeViewsData.php
    @@ -54,9 +54,11 @@ public function getViewsData() {
    +    $data['node_field_data']['promote']['help'] = t('A boolean indicating whether the node is visible on the front page.');
    

    EntityViewsData using StringTranslationTrait so why not $this->t()?

    I see the file uses t() elsewhere so opened a follow-up #2667346: EntityViewsData extending classes should use $this->t() but many use t().

  2. +++ b/core/modules/views/src/ViewsDataHelper.php
    @@ -114,8 +114,17 @@ public function fetchFields($base, $type, $grouping = FALSE, $sub_type = NULL) {
    -                  if ($string != 'base' && $string != 'base') {
    

    heh

Committed/pushed to 8.1.x and cherry-picked to 8.0.x. Thanks!

  • catch committed b51c12a on 8.1.x
    Issue #2509722 by NickWilde, dawehner, Lendude, rakesh.gectcr, Nitesh...

  • catch committed 8c4b00e on 8.0.x
    Issue #2509722 by NickWilde, dawehner, Lendude, rakesh.gectcr, Nitesh...
dawehner’s picture

EntityViewsData using StringTranslationTrait so why not $this->t()?

Well, because I copy and pasted it ...

xjm’s picture

Issue tags: +Usability

Yay! Great work, everyone. Really glad to see this fixed.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.