Problem/Motivation
For the Configurable Help Topic system (see #2351991: Add a config entity for a configurable, topic-based help system and my sandbox project https://www.drupal.org/sandbox/jhodgdon/2369943 which is developing it), I need a way for a config entity to have a "Body" field containing formatted text. hook_help() implementations also need a way to have a large chunk of HTML-formatted text as their return values.
The obvious way to handle the config entities and hook_help() return values is to have the "body" be one big HTML string, but this is problematic. The main reason is that strings on config entities and strings that are returned from hook_help() need get translated (on localize.drupal.org if they are provided in Core), and having large translated strings with HTML tags in them poses problems:
- Translations get invalidated if any part is changed. So it is better if the strings are shorter rather than the entire text of the "body", so that if one sentence changes, only one shorter string is invalidated.
- Long strings take longer to translate and review for translators, and are harder to get right (one error in one place means it can get rejected).
- It is difficult for the translators to get all the HTML tags right. They can get corrupted or become unmatched.
So, rather than having one big string with HTML embedded in it, it is preferable to have shorter strings that are about a paragraph in length or so, and preferably without most of the HTML tags.
As a note, hook_help() implementations are currently doing things like:
$output .= '<p>' . t(something) . '</p>';
return $output;
which removes the HTML tags and shortens the strings, but is ugly.
You cannot use a strategy like that that in a config entity, though -- you need some kind of a system for splitting up the string and abstracting out the HTML tags.
Proposed resolution
Create a plugin system for TextSection plugins, and a TextSections render element to render them.
Each TextSection is something like "paragraph", "header", "sub-header", or "bullet list item". The plugin data contains the text to go in the "section", and the plugin class knows how to render itself into HTML, as well as combine itself with other sections (for instance, consecutive bullet list items form a UL list when rendered into HTML, and consecutive description list name/value items form a DL list in HTML).
A developer can make a render array that looks like this:
$build['body'] = [
'#type' => 'text_sections',
'#sections' => [
[
'#section_type' => 'heading',
'#text' => ['#markup' => t('Greetings')],
],
[
'#section_type' => 'paragraph',
'#text' => ['#markup' => t('Hello, world.')],
],
],
]
and it will be rendered into something like:
<h2>Greetings</h2><p>Hello, world.</p>
In a config entity, you can have a schema that looks like this:
my_config_entity:
type: mapping
mapping:
body:
type: sequence
label: 'Body'
sequence:
- type: my_config_body_section
my_config_body_section:
type: mapping
mapping:
type:
label: 'Type'
type: string
text:
label: 'Text'
type: label
(Or, the last one could be type: text_format to allow some HTML like A tags in the individual "sections".)
And then in your individual entity config YML file, it comes out looking something like this:
body:
-
type: heading
text: Greetings
-
type: paragraph
text: 'hello world'
Alternative Proposals and Similar Things
- Markdown
- A separate issue #2698035: Add markdown system for HTML-formatted text is exploring the alternative idea of using some type of markdown to achieve the same goal. So far it is just discussion and doesn't have a patch. Also, Markdown alone doesn't "chunk" the output, so you would need to have authors manually choose to "chunk" the markdown into paragraph-sized bits to achieve the goals here.
- Paragraphs module
- For content entities using Field API, the contributed https://www.drupal.org/project/paragraphs (Paragraphs module) can be used to break longer HTML content into chunks. However, it doesn't work for hook_help() or for config entities.
- HtmlTag
- There is an existing RenderElement called HtmlTag. It doesn't work for this use case:
- Within each chunk, this use case requires some HTML tags (like A tags, EM, STRONG, etc.). HtmlTag doesn't support nested tags or passing in a render element, and has built-in XSS filtering, which might break some allowed HTML (in this proposed patch, we use a text format / filter chain for within the chunks).
- Also you want some of the paragraph-level elements to be joined into UL, OL, or DL lists (that's the "group" functionality). HtmlTag doesn't do that, and it cannot. Individual render elements don't have context like "what was the previous one?", so it seems that to do this kind of list joining, you would want to pass the chunked content into a single render element (like the TextSections one introduced in this patch), and then use the individual TextSection plugins that know how to join themselves with other elements.
- Component library
- This is being discussed on #2702061: Unify & simplify render & theme system: component-based rendering (enables pattern library, style guides, interface previews, client-side re-rendering) -- but (a) it doesn't seem to cover the use case of this issue and (b) it doesn't seem like it's going to get resolved in anything like a timely manner -- it's a Big Sweeping Architecture Change.
Remaining tasks
a) Take the part of the patch from #2351991-145: Add a config entity for a configurable, topic-based help system that has the TextSection/TextSections stuff in it, and make a smaller patch here from just that. [Patch is awaiting review now]
b) Convert a hook_help() implementation in Core to use this, as an illustration of how much nicer it is than returning HTML strings. [this is in the patch]
c) Use this in the Configurable Help sandbox as an illustration of how to do it in a config entity [already done, this patch came from that sandbox].
d) Write a change notice [draft is done]
e) Decide whether this approach or #2698035: Add markdown system for HTML-formatted text is better, and get one of them committed to core.
User interface changes
None.
API changes
API addition: New TextSection plugin system and TextSections render element. No changes to existing APIs.
Data model changes
No.
Comment | File | Size | Author |
---|---|---|---|
#23 | interdiff.txt | 8.75 KB | jhodgdon |
#23 | 2697791-23.patch | 37.53 KB | jhodgdon |
Comments
Comment #2
jhodgdonHere's the patch.
Comment #3
bojanz CreditAttribution: bojanz at Centarro commentedIn general, the idea of help being stored in a config entity, and the body being split into multiple paragraphs for easier translation makes total sense.
The plugin architecture as discussed would work.
However, I am wondering if we can simplify it by adopting Markdown.
The reason why there's a plugin architecture rendering lists and headings is because HTML is long and painful for translators.
Markdown reduces that cost to only a few extra characters ('# My heading') and is already used in many help and documentation contexts.
The previous Markdown discussions (like #2191525: [PP-1][policy, no patch] Extend Markdown coding standards to support API module (DOXYGEN)) didn't go anywhere, but the fact remains that there's a good Markdown standard (CommonMark) and a great library implementation (thephpleague/commonmark, in Drupal via the CommonMark module).
So in that idea we keep the config entity with multiple paragraphs, just use a non-HTML markdown-like format.
EDIT: jhogdon tells me that CommonMark doesn't support DT/DD lists (we could try and see if we can get that into the standard), and that drupal.org uses asciidoc (which is similar but has no PHP library). I'm not married to the actual markup flavor used.
Comment #4
jhodgdonI'm not sure that adopting some type of markdown would work well... The issues with HTML strings are:
a) The tags can get corrupted -- using a markdown syntax would remove those, but you'd have to still have the right sequence of === characters and other formatting characters. Better, but not necessarily perfect. With the plugin system here, the "type" of the text section is not even part of the translated string, so all you have is the text itself.
b) The strings are too long. I do not see how using a markdown syntax solves this problem? You want to split the text into paragraph-length chunks rather than translating the whole "body" as one string. I've clarified this part of the issue summary.
There might be a way to get around (b) with markdown, but I think it might be tricky.
Comment #5
bojanz CreditAttribution: bojanz at Centarro commentedI am assuming we would keep the paragraphs even while using markdown. That parts stays, only the plugin-based rendering would be removed.
Disclaimer: I have no need to sidetrack this, just wanted to put another option on the table.
Comment #6
jhodgdonYes, thanks for bringing up that alternative. Maybe we should open a separate issue to discuss its merits, so that we can compare what would be needed for that, and as an example, what the same help.module hook_help() implementation would look like?
Comment #7
jhodgdonAlso, Adding another note about this as applied to config entities to the issue summary and will add to the draft change record too.
Comment #8
jhodgdonI've added a related issue to discuss whether some type of markdown would be a viable/better alternative. @bojanz or someone else -- I wasn't sure how exactly that would work in a few places, so an issue summary update there would be helpful. Thanks!
#2698035: Add markdown system for HTML-formatted text
Also added a bit to the issue summary here (and on the other issue) about hook_help() as a prime user of this system, whichever way we go.
And just to be clear, if Markdown is a preferable way to do this over plugins, as long as it is viable and gets done, I will be happy. :) Maybe once the idea of markdown is filled in a bit better, we can compare what it would look like to a developer... the patch here has a sample of what this idea would look like in a hook_help() implementation, and the issue summary also shows what it would look like in a config entity (my config help sandbox, mentioned in the summary, also has more samples of both of these). We should adopt whichever system is better for developers, meaning flexible enough to cover most use cases and address the issues in the Problem statement, and easiest to develop with.
Comment #9
jhodgdonAdding links to related/competing proposal for markdown to summary.
Comment #10
jhodgdonDoes anyone want to review the patch here? Or comment on this approach vs. #2698035: Add markdown system for HTML-formatted text?
Comment #11
andypostGenerally approach looks awesome! +1 to that
Also it looks like the same going to be happen in
#2702061: Unify & simplify render & theme system: component-based rendering (enables pattern library, style guides, interface previews, client-side re-rendering) and #1804488: [meta] Introduce a Theme Component Library
suppose heading should point its type (1..6)
Comment #12
jhodgdonRegarding headings, I just called them "heading" and "sub-heading", and they're (by default) H2 and H3. If you are trying not to be HTML-specific, I think that is more semantic. I'll take a look at those other issues anyway; it could be we can close this one as a duplicate.
Comment #13
jhodgdonI took a look at the related issues from #11. So we have three issues:
a) #2702061: Unify & simplify render & theme system: component-based rendering (enables pattern library, style guides, interface previews, client-side re-rendering) -- as I read it, the problems it is trying to solve are: "Rendering and theming are too intertwined, there is still too much happening in ad-hoc PHP processing steps, and there is too much flexibility in the variables and properties in render elements and theme templates".
b) #1804488: [meta] Introduce a Theme Component Library -- as I read it, the problem it is trying to solve is: "The Drupal admin UI is too ad-hoc, and it would be better if it was built up from a set of UI components".
c) This issue (or the alternative #2698035: Add markdown system for HTML-formatted text) -- the problem it is trying to solve is: "There is not any HTML-independent way to specify a block of formatted text in small, translatable chunks".
So I don't think (b) will help us -- it is about UI components, not chunks of formatted text. If (a) gets in, it would mean redoing this patch so it renders down into components instead of render elements, and so that instead of providing a render element for #text_sections it would instead provide a component... That would probably be easy enough to do. But I think we still would need the plugin system here in order to have a defined set of text chunks... I don't think you would want to use bare components for that?
Thoughts?
Comment #14
jhodgdonOne note on my most recent patch, which I'm adding here so I hopefully wont' forget it: I think that the functionality of "Make an options list for selecting section types in the UI" should be added to the plugin manager instead of being (as it is now) in my (sandbox) Config Help module buried in a form builder. That way it could be used by other UIs. Probably the part that takes an array of sections, each having type/text on it, and converts it to a render array should also be put into this patch and centralized. Then these two things could be (a) centralized and (b) unit tested.
Comment #15
jhodgdonActually I maybe should set this to Needs Work for #14. Then I for sure won't forget to do this. ;)
Comment #16
jhodgdonHere's an update for #14. I decided that the options list made sense to put on the plugin manager, but not the view builder. The view builder I have for help topics is doing other stuff (token replace) at the same time as it builds the render array, and it would be more trouble than it is worth to separate that out, so I don't think a generic one can be made that will actually be useful.
Comment #17
jhodgdonNote: I've been following the discussions on #2702061: Unify & simplify render & theme system: component-based rendering (enables pattern library, style guides, interface previews, client-side re-rendering). I (a) don't think it's going to cover the use case of this issue and (b) don't think it's going to get resolved in anything like a timely manner -- it's a Big Sweeping Architecture change and I am not even sure that it would cover this use case when/if it is resolved.
Meanwhile, we have a present need for the configurable help project, and for the existing hook_help() [to get rid of all the HTML tag mess that it currently has, with one example of this in the present patch]. We could go with this patch's approach, or the Markdown alternative proposed and as of yet not patched on #2698035: Add markdown system for HTML-formatted text -- but we need something.
So... a review of this patch, and some thoughts on the merits of this approach vs. a Markdown approach, would both be appreciated. I'd like to get this resolved sooner rather than later... Thoughts?
Comment #18
larowlanIs a future enhancement the ability to define the sections in yml?
Cause that would make it way easier.
Would be great if we provided a method for that.
Comment #19
jhodgdonThat is an interesting idea... The section plugins in the present patch have code in their plugins. Some of it is simple (just adding a prefix/suffix to the HTML output, which could be taken care of by the YML having prefix/suffix elements in it, I suppose). But some of it is more complicated, like the code that lets bullet, numbered, and description list elements make groups.
To tell you the truth, I'm not sure what additional sections would be needed.
Comment #20
larowlanSorry, I wasn't clear enough - these are the bits I want to define in YAML, not the plugins. I agree there aren't many more we will need.
But I think it will be easier to define the actual sections in YAML than this array format.
Cheers
Comment #21
jhodgdonOh, you mean instead of hook_help(), use YAML. Hm...
So, I have examples of sections in YAML in the Configurable Help sandbox project... check out this example:
http://cgit.drupalcode.org/sandbox-jhodgdon-2369943/tree/config/install/...
In this config entity, the YAML with sections is in the 'body' config property. So... I don't think a method like TextSectionBase::fromYamlFile('file name') would help very much, because it's embedded in the config a few levels down.
The code I currently have for taking the 'body' property and converting it to a render array like what is in help_help() that you cited in #20 is in:
http://cgit.drupalcode.org/sandbox-jhodgdon-2369943/tree/src/HelpViewBui...
And I decided not to put this into the proposed Core patch here, because besides just doing a straight conversion, it's also dealing with some cache tags, and doing a token replace. Plus, the HelpViewBuilder doesn't have the YAML per se to deal with, but instead it has the PHP array that it's parsed into.
So... not sure what to do about that?
Comment #22
Wim LeersOn the subject of how this relates to #2702061: Unify & simplify render & theme system: component-based rendering (enables pattern library, style guides, interface previews, client-side re-rendering):
+1 on both counts. Especially the "timely manner" one. If this will eventually turn out to be solvable using that, then great, but it's far too uncertain at this time.
And a review.
public $suggestedWeight
?Nit: 80 col alignment.
Then it's still not plain text… So it still allows for arbitrarily complex HTML.
You might as well use
#lazy_builder
.I've never seen an underscore-prefixed child.
It's better to use
\Drupal\Core\Render\RendererInterface::addCacheableDependency()
.Should this be typehinted to
array
?Shouldn't this be
<pre><code>
?This is then effectively a very limited theming system…?
I'm also reminded of
\Drupal\Core\Render\Element\HtmlTag
.Comment #23
jhodgdonThanks very much for the review!
Regarding items 3 & 9... What I needed for the Config Help project was the ability to "chunk" the body of help down to paragraph sizes (mostly for translation purposes, see issue summary). But within each paragraph, at least in the Config Help use case, you'd still want to be able to embed links, emphasis, strong, etc. -- in other words, a limited set of HTML tags but not paragraph-level tags.
To do this in Config Help, I introduced a text format for within the chunks, which allows A and EM and STRONG but not paragraph-level tags (of course an admin could override the text format, or when editing help use a different one, but you can hope that they won't). Then in the render array that config help creates, each #text in a section chunk is of type #text_format. In the example of hook_help() in help.module that is in this patch, also you want some A tags in the text of each paragraph or DD item. It gets rid of a lot of HTML tags, but not all of them -- you still want links in help.
Regarding HtmlTag -- I tried to use it, but it didn't work for this use case at all. HtmlTag requires a pure string for the value of the tag, and it doesn't support nested tags or passing in a render element -- you'd have to pre-render the element value. Plus HtmlTag has built-in XSS filtering, which kind of defeats the purpose of having filtered text in the first place (and might break the HTML that someone wanted to put into the chunk). Also, you want some of the paragraph-level elements to be joined into UL, OL, or DL lists (that's the "group" functionality). HtmlTag doesn't do that, and since individual render elements don't have context like "what was the previous one?", it seemed better to have a TextSections render element that knew about context, and then these individual TextSection elements that know how to join themselves with other elements.
So... Yeah, this doesn't completely abstract away the HTML (sorry! I know you'd like to do that. ;) ). It's just meant as a way to be able to break long text up into paragraph-size chunks, for reasons mostly of translatability, and get rid of some of the HTML tags, abstracting them slightly.
Regarding the rest... thanks! Here is a new patch, plus or minus a few things:
item 1: This is used for sorting in the plugin manager. Made the docs clearer about what it is for, instead of changing the name of the annotation property.
item 4: I am not sure about lazy_builder... https://api.drupal.org/api/drupal/core!lib!Drupal!Core!Render!Element!Re... says:
That doesn't seem to fit the use case here -- the builder (prerender or lazy) needs to process the #sections element in the render array... ??? So I left this alone. Open to suggestions. ;)
Anyway, here's a new patch and interdiff. My config help tests still pass (as well as the one in this patch, which was slightly updated for items 5 and 8).
Comment #24
jhodgdonAnyone want to review the latest patch? Thanks! I'll also hit Retest.
Comment #25
miro_dietikerI was pointed here from our Paragraphs issue queue.
Agree, HTML in code is ugly. Wrapping is a problem and the HTML code readability is hard.
HTML5 tried to address this as a first step with being much more semantic.
Also mixing code with rich help content is blocking help from independent content driven evolution.
Not sure we should go into this direction. There are many overlaps to other concepts. And we are adding a new layer of complexity that is limited to the config use case.
The proposals in YAML are far from native in content creation / editing. Dunno if autocomplete of the type system would ever work. Help editors will always need to lookup the defined types, ...
The content is still fragmented and far from the minimalistic definition something like Markdown provides.
Also, if we look at our code documentation standards, even annotation, we managed to establish a content oriented definition with less clutter.
I think something like Markdown would be much more near to the help content creator.
We sometimes ended up outsourcing things into small Twig template files where we could also add conditions. Mind that contextual help needs conditions and plain YML decoupling won't make us happy in the long run. This direction could be better aligned with how core is moving forward?
BTW: With evolving editability though UI we also thought about introducing a Twig plugin for CKEditor for applications such as editing a mail with conditions.
Independently of the current scope of help:
It always made me unhappy how the Drupal entity type + field type system was limited to a stiff structure and needed a site builder to add variations. For many customers and use cases, real pieces of content are more dynamic in structure and that's why pressure on WYSIWYG is raising to surmount the other limitations. It results in a mismatch between CMS semantic awareness (not knowing much more than "it's HTML, Browser!") and requirements of the modern web for multi channel output with different technologies including REST based decoupling and much more.
This limitation led us through multiple steps of contrib invention of less stiff systems such as Dynamic Properties, Collect, and Paragraphs.
Thus agree, Drupal should step a big leap forward in this area.
IMHO we should not reinvent the wheel and build on top of something that is existing.
Comment #26
jhodgdonPersonally, I am not convinced that using Markdown is better.
For one thing, for translatability (see issue summary), we need the text in config to be "chunked", and Markdown is not chunked.
Second, I am not convinced that Markdown syntax is more abstracted than HTML. You're just replacing one syntax that provides markup with another that provides markup.
Third, Markdown may be easier for some authors to use than HTML, but not others (for instance, I have been writing raw HTML since the 90's, so it is very quick for me, while I would have to think about Markdown syntax because it is less familiar).
So... I don't think Markdown by itself solves the problems in this issue, and I am not convinced that a "chunked markdown" syntax, as is being discussed on the related issue #2698035: Add markdown system for HTML-formatted text, is better than this plugin system.
Comment #27
jibran#19 make sense to me. I think there is not a lot of logic involved here so yml discover of plugins can work here.
Overall, I was not thrilled about this patch until I saw hook_help. I agree with the point of reinventing the wheel. We are introducing the new system here which is useless other then using it in hook_help and in all honesty we can also achieve this with
\Drupal\Core\Render\Element\HtmlTag
we'd need a new render element for dt and dd.Patch looks good tests coverage is great.
Comment #28
jhodgdonIt is not just for hook_help(). My real motivation for proposing this is the Configurable Help system (see issue summary). The fact that it can also be used for hook_help() is a bonus.
Someone still needs to either mark this patch Needs Work or RTBC so we can move on it one way or the other. Thanks!
Comment #29
jhodgdonI guess I am not understanding #27, whether (all things considered) you like the patch or not?
And I cannot use HtmlTag as it is now to have this functionality. See #23, where I discussed that point.
Comment #30
jhodgdonAdding a section on Alternatives to the issue summary.
Comment #31
jhodgdonI'll make one more appeal here... Anyone want to please mark this as RTBC or NW? I'd like to move forward, one way or the other, with my configurable, topic-based help system... Thanks!
Comment #32
miro_dietikerWhy is this wrapping needed at all? Why can we not just implicitly treat an array like that? This will be repetitive everywhere.
So we don't support a third level ever?
I see you simplify flat list building, but i think we should support nesting inside numbered. I can't see from tests - do we?
Thus at least back for those items.
Reviewing this again, i was thinking about why we are not simply introducing HTML5 tags with proper attributes that we are converting?
Transformation could be much less drupalism. An example:
We could wrap things automatically through t(). No need for the user to care that still need to do with your proposal.
This could be simplified to an easy HTML notation such as:
The Help module displays explanations for using each module listed on the main <a url="help.main">Help reference page</a>
To be discuss is, if we would redefine the a tag, use url or href, or define a new tag.
Combined with a proper HTML schema, editors could support content creation with allowed attributes, ... with standard tools.
Comment #33
jhodgdonThanks for the review in #32!
A few thoughts:
1. I do not understand what you are asking for in this item, can you clarify please?
2. I made the minimal set of section types that I needed for Help topics (both hook_help() and configurable help [config help is not in this patch, however]. Neither of these two use cases needs anything beyond 2 levels of headings, and I am not really in favor of making code that isn't needed, as it leads to code bloat. If more levels of headings are needed for another use case, they can easily be added when they're needed, rather than adding them when they're probably not needed..
3. Regarding nesting, nested lists would not work with an editing screen in config help (which was really my main purpose for this -- to use in https://www.drupal.org/sandbox/jhodgdon/2369943 -- the configurable help sandbox project). Or at least, I can't see how they would. And since they would break my main use case, I'm not willing to add them to this patch.
So if this has to be Needs Work for 2 & 3, then it's Won't Fix. Adding nesting will break my only motivation for doing it, and adding more headers is code bloat that I think we don't need, at least for now... I'm slightly more willing to add more headers than nesting, but the nesting would break my other use case so it's a no-go.
Regarding your other suggestion about using HTML5... The main purpose (for me) in adding this system is to support Configurable Help, and as a side effect, to allow removal of most of the HTML markup from hook_help(). Doing what you are suggesting would (a) not satisfy my YML and translation-break-up use cases, and (b) not satisfy the goal of abstracting/removing HTML from hook_help(). At least, if it would, I don't see it.
So, I'm going to mark this back to needs review one more time, to hopefully get more opinions. If others agree with the review of #32, then let's just mark both this and #2351991: Add a config entity for a configurable, topic-based help system as Won't Fix, and I'll go find something else to work on. Thanks!
Comment #34
miro_dietiker1. your two hook_help examples build $section and then repeatingly need to wrap that into $build with type text_sections. Can we not make hook_help return text_sections - thus auto-wrapping it on the caller?
Yeah sorry, i was more focussing on hook_help, not config, since only hook_help is there.
2. 3. I was just asking for clarification, i understand your points now. The main point was 1. that would seemingly simplify usage of your new structure in hook_help.
Keeping needs review for those others' opinions. :-)
Comment #35
jhodgdonWe cannot really change hook_help() return value right now. It supports either (a) a string or (b) a render array. It would be a non-backwards-compatible change to assume that an array return should be marked in text sections. That would be Drupal 9 material.
Comment #36
jhodgdonActually, I'm just going to mark this as Won't Fix, and do the project in contrib, if at all. I've been trying to get the config entity help system going for more than two years, and it's just too difficult to get things done in Core. I've lost my energy for it... Sorry.
Thanks everyone who reviewed this patch -- it is much better than it was at the beginning, due to your reviews!