Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
Comment | File | Size | Author |
---|---|---|---|
#79 | interdiff.txt | 1.54 KB | dawehner |
#79 | 2509722-79.patch | 4.79 KB | dawehner |
#76 | interdiff.txt | 2.31 KB | dawehner |
#76 | 2509722-76.patch | 4.75 KB | dawehner |
#71 | 2509722-70.patch | 4.99 KB | dawehner |
Comments
Comment #1
NickDickinsonWildeComment #2
NickDickinsonWildeComment #3
NickDickinsonWildeoops only uploaded 1/3 of the patch! full patch now.
Comment #4
NickDickinsonWildetagging for attention ;)
Comment #6
snehi CreditAttribution: snehi as a volunteer and at Publicis Sapient for Publicis Sapient commentedHi @Nick i am having query on vocab used in
Can we change the word
Otherwise patch is looking good.
Comment #7
NickDickinsonWilde@snehi, Do you think
or
would be better, or got any suggestions?
thanks
Nick
Comment #8
snehi CreditAttribution: snehi as a volunteer and at Publicis Sapient for Publicis Sapient commentedI think
Comment #9
NickDickinsonWildeSlight further change; conforms to the style/standard of the other descriptions better:
Sound good to you?
Comment #10
snehi CreditAttribution: snehi as a volunteer and at Publicis Sapient for Publicis Sapient commentedWhy Description is on second line might we due to which it is giving error.
Comment #11
Nitesh Pawar CreditAttribution: Nitesh Pawar at Trigyn Technologies Ltd commentedI 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."
Comment #12
NickDickinsonWildeIt 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?
Comment #13
snehi CreditAttribution: snehi as a volunteer and at Publicis Sapient for Publicis Sapient commentedMy mistake. This is RTBC now.
Comment #14
snehi CreditAttribution: snehi as a volunteer and at Publicis Sapient for Publicis Sapient commentedAdding Screenshot for the reviews.
Comment #15
tstoecklerThis formatting is very non-standard in core. Can we just collapse that to one line please?
Comment #16
NickDickinsonWildeThose lines were going over the 80 char - and as per coding standards
I figured those should be split to decrease that, is that not the case?
Comment #17
Nitesh Pawar CreditAttribution: Nitesh Pawar at Trigyn Technologies Ltd commentedcollapsed description to one line
Comment #18
ameymudras CreditAttribution: ameymudras as a volunteer commentedIts working good now. Looks like RTBC to me
Comment #19
tstoecklerRe #16: This fits into the
(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.
Comment #21
NickDickinsonWildeCI error probably...
Comment #22
charginghawk CreditAttribution: charginghawk at Genuine commented#17 fixed this issue for me - Title had "Error: missing help", but no more.
Comment #23
NickDickinsonWildeComment #24
NickDickinsonWildeComment #25
Sagar Ramgade CreditAttribution: Sagar Ramgade as a volunteer commentedLooks like RTBC to me.
Comment #26
xjmSo, 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.:
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.
Comment #27
xjmActually 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. ;)
Comment #28
xjm@NickWilde asked what the correct step forward in this issue would be. I think it will look something like this:
Comment #29
xjmAlso, it will need tests for whatever fallback behavior we do implement. :)
Comment #30
NickDickinsonWildeWell 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.)
Comment #31
NickDickinsonWildeComment #32
xjmThat 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?
Comment #33
NickDickinsonWildeShould #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.
Comment #34
LendudeNumber of other issues about this, with some more discussion about what the right fix is for this.
Comment #35
xjmNice 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.
Comment #36
NickDickinsonWilde@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.
Comment #37
LendudeOh 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
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.
Comment #38
NickDickinsonWildeNode 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)
Comment #39
rakesh.gectcrhttps://www.drupal.org/node/2597231
Comment #40
xjmOK, now I think we're ready for a usability review, since we are only changing the Views UI. :)
Comment #41
tstoecklerI 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.
Comment #42
dawehnerFirst, 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
Comment #43
xjmThanks @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.
Comment #44
dawehnerFair 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 ...
Comment #45
xjmBTW:
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.
Comment #46
tstoecklerHmm... 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 :-/
Comment #47
dawehnerSo 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.
Comment #48
NickDickinsonWildeSo 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.
Comment #49
xjmThis 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!
Comment #50
xjmUpdating the summary to clarify the remaining tasks.
Comment #52
xjmOops, fixing which screenshots are where.
Comment #53
NickDickinsonWildeSince #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?
Comment #54
xjm@NickWilde, yep, that's part of it. Quoting my notes from the summary:
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.
Comment #55
Bojhan CreditAttribution: Bojhan as a volunteer commentedIf 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.
Comment #56
xjmOkay, thanks @Bojhan!
So based on @Bojhan's recommendation, let's
removeremove the redundant entries innode_views_data_alter()
entirelynode_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.
Comment #57
xjmLet's remove these two lines since they are mostly redundant.
Instead of this, fall back to an empty string.
Comment #58
rakesh.gectcrComment #59
rakesh.gectcr@xjm
I have done the changes you mentioned.
Comment #60
LendudeHmm 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.
Comment #61
snehi CreditAttribution: snehi as a volunteer and at Publicis Sapient for Publicis Sapient commented@Lendude
Can you please provide the interdiff ?
Comment #62
Lendude#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
Comment #63
dawehnerI'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 ...
Comment #64
xjmYeah, 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.
Comment #65
LendudeSounds 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.
Comment #67
LendudeSo this now has tests
Comment #68
dawehnerI'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?
Comment #69
dawehnerSo yeah let's drop the requirement to have a help text. It seems to cause more harm than good.
Comment #71
dawehnerOne tiny detail ...
Comment #72
dawehnerComment #73
dawehnerComment #74
LendudeCouple of nitpicks:
Truncate at 80 characters.
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)
Comment #76
dawehnerThank you @Lendude for the quick feedback
Comment #78
Lendudevalue and test value don't match
do we also want to remove this double check on the same string while we are at it?
Comment #79
dawehnerYeah, I guess its right to drop this little gem.
Sure here it is
Comment #80
Lendude@dawehner nice! Looks good to me now.
Manually tested #79 and looks good there too.
Comment #81
damiankloip CreditAttribution: damiankloip at Tag1 Consulting commented+1
Comment #82
catchThis looks like a good solution to not cluttering the entity forms but keeping descriptions in Views where they're useful.
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().
heh
Committed/pushed to 8.1.x and cherry-picked to 8.0.x. Thanks!
Comment #85
dawehnerWell, because I copy and pasted it ...
Comment #86
xjmYay! Great work, everyone. Really glad to see this fixed.