| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | install system |
| Category: | task |
| Priority: | normal |
| Assigned: | yched |
| Status: | needs review |
| Issue tags: | Needs tests, Novice |
Issue Summary
For the "Page" content type that comes pre-configured with a normal Drupal install, I advocate for using the "long text" field type instead of "Long text with summary" for what used to be the body field.
The Page use-case wouldn't likely need a summary. And the UI with the long text with summary is still somewhat problematic, with a confusing "edit summary" link next to the Full text label. I think that having an alternative approach to handling a text field pre-installed is of great benefit. And the fact that the data-entry UI would be so much simpler is a real plus.
On the support side of things, when people are confused with the full text with summary UI we could point them to the Page content type and easily show them an example of a different solution that might get them what they want with a simpler UI. It's a lot easier to tell people, "Go to node/add/page" then it is to give them instructions on adding a new content-type and adding a field.
Comments
#1
@Shai
I'm not understanding this part
This would be if it's left as it is? Or you saying when they ask about the Article content type and are confused?
I do however agree that the Page content type should just include the long text. As the use case (hope I'm using this term properly) is as a "static page" versus a content type where the summary is necessary.
#2
@WebWeaver64,
Yes, that is what I'm talking about.
Shai
#3
I'm bumping this...
The standard.install makes all kinds of distinctions between "Article" and "Page" (soon to be "Basic Page") in order to help the user.
These include:
Article is promoted to front by default while Page is not.
Article shows post info by default while Page does not.
We need to make yet another distinction in the install profile between these two pre-configured content-types.
The suggested use-case for "Page" simply does not demand the "Long-text with summary" text widget and it just adds confusion. Before Fields in Core, we couldn't make this distinction. Now we can and we should change the "Basic page" content type to be a "Long-text" field without the summary business.
Shai
#4
I'm changing this to "bug" as in "user interface" bug. So much about D7 is introducing UI improvements and I think this issue is important in that light, and easy to implement.
#5
Sorry,but this seems by design? A page has no need for a summary.
#6
@bojhan,
Yes, I agree that a "Basic page" (changed from "Page" on a recent commit) does not need a summary. But it has one. I'm advocating taking it away.
Shai
#7
Makes sense to me.
I was going to write that since these are actually two separate field types, and the node module still has some special handling for the body field (which is the "long text with summary" type), this might actually get a bit tricky, since essentially Basic Page wouldn't even have a "real" body anymore.
However, I also just noticed that within "long text with summary", there is a widget option to hide display of the summary textarea. So that might be another possibility - to use the same underlying field type for both, but do not configure pages to show that the summary on the node edit form.
#8
@David_Rothstein or others, is it written somewhere... Or could you give me some direction to how to most easily find: in what ways node.module maintains special handling for the body field? Maybe just searching on $body will do it?
In addition, were there discussions about the future of the body field when the push toward fields-in-core was happening?
If 8 is two years away, it would be a shame not to leverage the power of fields-in-core with D7.
Shai
#9
I'm not sure where it is in the code (probably in a few places), but see the attached screenshot.
That "body field label" section is a carryover from Drupal 6, and it still works as described - just now, it is hardcoded to work with that particular field, at least as far as I remember.
I don't remember the exact history, but I think it may have just been an attempt to keep the "convert body to a field" patch down to a minimum size without ripping apart the UI as well - it was kind of a monster patch :)
#10
I was thinking about this and I think it might be pretty easy.
I agree that that there are bigger issues relating to the sea change of fields-in-core. There will be some tough work ahead.
But focussing on just this issue, the pre-configuration of the "standard" Drupal install, it should be pretty easy. What we are talking about here gets entirely implemented by Drupal's install script upon an initial installation of Drupal. That's it.
So this is how I imagine the install script working:
* create content-type with human name "Basic page" and machine name "Page"
* remove the body field
* add field_body of type "Long text"
Is there any reason why the "remove body field" part couldn't be written into the install script?
Am I missing something?
Shai
#11
No, I don't think it would be hard at all - I was just wondering about the user experience of shipping with a content type that had something kind of like the body field, but wasn't actually the same field that is special-cased in the UI, so the controls wouldn't work for it.
However, I just found out that there is ongoing work at #553306: Make nodes have no body field by default. Remove deprecated APIs for body field to get rid of the node body special case altogether, in which case that problem would go away completely!
#12
marking as duplicate of http://drupal.org/node/553306
#13
The progress at http://drupal.org/node/553306 is awesome!
It still doesn't take care of this problem, however.
Here is the current code starting at line 225 of profiles/standard/standard.install:
// Insert default user-defined node types into the database. For a complete
// list of available node type attributes, refer to the node type API
// documentation at: http://api.drupal.org/api/HEAD/function/hook_node_info.
$types = array(
array(
'type' => 'page',
'name' => st('Basic page'),
'base' => 'node_content',
'description' => st("Use <em>basic pages</em> for your static content, such as an 'About us' page."),
'custom' => 1,
'modified' => 1,
'locked' => 0,
),
array(
'type' => 'article',
'name' => st('Article'),
'base' => 'node_content',
'description' => st('Use <em>articles</em> for time-specific content like news, press releases or blog posts.'),
'custom' => 1,
'modified' => 1,
'locked' => 0,
),
);
foreach ($types as $type) {
$type = node_type_set_defaults($type);
node_type_save($type);
}
Something has to change there to make sure that the "Basic page" content type will be created with "long text" and not "long text with summary." In fact, maybe after 553306 is committed, it's possible the install script will fail. Or more likely, both "Article" and "Basic page" will get created without any "body" ish field at all.
Maybe the title of this issue should be changed to, "Setting body fields for pre-configured content-types now that body field is no longer a special case."
Shai
#14
Nothing is actually broken here so it is a task
#15
#1378350: Clean up the "Long text and summary" field will work nicely with this.
#16
I started a duplicate #1396170: Change the default body formatter for Page content type from 'long text and summary to 'long text' which should be handled here:
From http://drupal.org/node/1378350#comment-5436402
From http://drupal.org/node/1378350#comment-5436484
#17
Kick-starting this.
Not particularly pretty, but getting the job done.
Apparently, Node module (or rather the entire system) still has a pretty hard dependency on a 'body' field on nodes. Therefore, the actual field type has to remain the same, and we only adjust the widget + formatter.
#18
err, what? The last thing I expected was #17 to pass tests. ;)
#19
The patch in #17 implements what @yoroy quoted in #16; i.e.:
It removes the summary [widget] for the Basic page content type. (but also for book pages and forum posts)
#20
Oke, sounds good.
Also fixed the weird upper-casing of everything in the title.
#21
The patch in #17 looks good to me.
@@ -589,7 +592,7 @@ function node_add_body_field($type, $label = 'Body') {),
'teaser' => array(
'label' => 'hidden',
- 'type' => 'text_summary_or_trimmed',
+ 'type' => $widget == 'text_textarea_with_summary' ? 'text_summary_or_trimmed' : 'text_trimmed',
This is the only part that seems like it may be a bit confusing. I see what is happening here, but think that it could use a comment to explain the conditional statement.
#22
Regarding @Everett Zufelt's example in #21, I think the code would be a little more clear if we wrapped the condition
($widget == 'text_textarea_with_summary')in parens.#23
> Apparently, Node module (or rather the entire system) still has a pretty hard dependency on a 'body' field on nodes.
We might want to revise this. However, even w/o teaser it still is a 'body' field, so it's rather a good thing to treat it 1:1 the same way.
> Therefore, the actual field type has to remain the same, and we only adjust the widget + formatter.
Even this is a good thing, cause it should easily allow change widget and formatter if folks do want the teaser vice versa. Regarding the user experience, that's exactly the behaviour escoles and yoroy called for in #1378350: Clean up the "Long text and summary" field.
Plus:
- a bit more of obvious differentiation between 'article' and 'page' helps explaining the existence of these different node types to new users.
- it is a built-in nice simple demonstration about field types, widgets and formatters, flattening the learning curve.
#24
'Should' in issue titles is not productive :)
Needs work as per #21 and #22, adding Novice tag for that
#25
Wrapped the ternary statement with parens per #22, re-rolled patch
#26
#25: long_text_for_page_ct-626546-5447656.patch queued for re-testing.
#27
The last submitted patch, long_text_for_page_ct-626546-5447656.patch, failed testing.
#28
re-roll based on recent Drupal8 changes
#29
I think this makes sense. My only question is why we're not making the default widget
text_textareaand special-casing article, rather than (right now) special-casing everything else. I guess it comes down to whether a new custom node type should provide a summary or not by default.In either case, let's have someone test the patch manually and confirm the results for page, article, book, and forum nodes.
We also should add to the
node.moduletests covering this functionality. As sun alludes, the fact that the patch didn't fail any tests implies some missing coverage. At the least we want a unit test fornode_add_body_field(), but it might also be good to add some functional test coverage as well. Do we want to codify the kind of body found on the node types that ship with core?Also, let's find or file a followup for:
#30
#28: 626546.patch queued for re-testing.
#31
http://core.drupalofficehours.org/task/1036
After seeing this, I felt that this "task" still needs work. Anyways, i will test it manually myself once again and report the results here.
#32
#28: 626546.patch queued for re-testing.
#33
The last submitted patch, 626546.patch, failed testing.
#34
And the patch #28 needs to be re-rolled as well.
#35
Before applying the re-rolled patch:
This is what we had(screenshots attached). The "Field Type" was "Long text and summary" by default for Basic Page. And the "Widget" was "Text area with summary".
After applying the rerolled patch:
This is what we have(screenshots attached). The "Field Type" is "long text and summary" and "Widget" has now changed to Text area(multiple rows) for Basic Page.
Is this what we want? Or, are we looking to change default value for "Field Type" as well?
"Create Basic Page" is still the way it looked earlier(screenshots attached).
#36
Patch re-rolled in view of current changes in Drupal 8 core.
#37
#38
#39
+++ b/core/modules/book/book.installundefined@@ -40,7 +40,7 @@ function _book_install_type_create() {
- node_type_save($book_node_type);
Why is this removed? I think you meant to remove the node_add_body_field call instead
+++ b/core/modules/node/node.moduleundefined@@ -578,7 +581,7 @@ function node_add_body_field($type, $label = 'Body') {
+ 'type' => ($widget == 'text_textarea_with_summary') ? 'text_summary_or_trimmed' : 'text_trimmed',
So this sets the correct formatter but I can't see anywhere that the widget type is set in this diff.
#40
Regarding #35, there might be a bug in the server-side PHP or perhaps also client-side JS code for the summary handling. If the widget isn't "_with_summary", then the summary thingie shouldn't appear in the UI.
That said, did you re-install before testing?
Speaking of tests, we're still missing a test assertion for this. Should be dead simple to do:
// Verify that the Body field widget of the Basic page content type// does not use a summary widget.
$this->drupalLogin($this->root_user);
$this->drupalGet('node/add/page');
$this->assertNoText(t('Edit summary'));
These lines should be added to
core/profiles/standard/lib/Drupal/standard/Tests/StandardTest.php#41
Just a general little cheer in support of working on this issue: "Yay, thanks for working on this issue rkjha!".
#42
@sun: yeah, i did a fresh installation before testing. i do that every time.
Rerolled patch attached. Hope I have not done something wrong again.
#43
@sun: even after applying the patch, the summary thing still appears in the UI. And this is not only with me. Have a look at this http://core.drupalofficehours.org/task/1036.
#44
Added the tests as suggested by @sun
Lets see if this patch passes tests
#45
+++ b/core/profiles/standard/lib/Drupal/standard/Tests/StandardTest.php@@ -32,6 +32,11 @@ function testStandard() {
+ $this->assertNoText(t('Edit summary'));
Ouch, I'm sorry... this text doesn't actually appear in the markup that is generated on the server-side — it's injected via JS only, and the automated tests do not execute JS.
Can you replace this line with this one?
$this->assertNoRaw('text-summary');#46
Changes made to test as suggested in #45.
#47
According to what I have observed on testing it manually, it should not pass the test. Lets wait and see.
#48
The problem is that node_add_body_field uses the 'text_with_summary' field type for the Body field. This should be simply 'text_long' in the case of the page node type. So you either need to add an additional parameter for the field type, or you simply add a single $summary parameter which controls both the field type and the widget. What currently happens with the patch is that because the 'text_textarea' widget is not applicable to fields of field type 'text_with_summary' it is dismissed upon saving and the default ('text_textarea_with_summary') gets taken over. It is strange that there is no notice/exception when doing that, but that's not for this issue.
#49
1) At least we have working test coverage now :)
2) We cannot use a different field type, because the $node->body field can only have one field type, and there can only be one field with the name "body".
So... essentially, we need to make that widget work with the existing text_with_summary field type. I'd say, just ignore the summary value in the widget?
I.e.: Let's see what happens if we change the "field_types" definition in the annotation of Drupal\text\Plugin\field\widget\TextareaWidget to additionally list text_with_summary? @rkjha, can you try that?
#50
@tstoeckler: i tried changing that, but only to get strange results.
@sun: sure, i will. i think i need some more clarification, i'll ask for the same as soon as i am back to this work(maybe tomorrow).
#51
Helping you out until then :)
#52
@sun: I am unable to figure out what difference would the addition of "text_with_summary" in "field_types" make?
#53
Instead of using the widget name as the second argument, can we instead use a '$summary' boolean flag as eluded by @tstoeckler at #48
If $summary = TRUE, set the widget and field type accordingly, if $summary = FALSE, use the text_long, text_textarea type/widget.
#54
@rjkha, something like https://gist.github.com/4589799 is what I'm taking about
#55
As explained above that seems to cause additional problems. I assume there is lots of code around that assumes that $node->body is the 'text_with_summary' field type, so that I can do $node->body['und'][0]['summary'] and it works. In the long run, we should fix that in the one or the other way, but for now I think we have to stick with the existing field type.
#56
#51: drupal8.node-page-body.51.patch queued for re-testing.
#57
When node moves to EntityNG $node->body['und'][0]['summary'] will be gone and I don't think we should baby-sit anything that does that at the moment. What happens if $node->body doesn't exist? Surely code like that is only in tests.
#58
Code-wise, #51 looks good to me.
Whether it goes against some unknown Do's and Don'ts of Field API, or whether it is horribly broken in the UI, because this edge-case isn't covered by the field widget's existing tests, I don't know. :)
Let's see what @yched has to say. :) In case he doesn't respond soon-ish, we can ask @swentel, too.
Also, some manual testing of the Basic page content type would be nice; i.e., creating, editing, deleting content, managing fields and displays, as well as deleting the content type.
Oh, and let's also test what happens if you change the widget to the summary widget, and you enter some nodes with a summary filled in, and then change it back to the widget without a summary, and see what happens — i.e., the previously entered summary value will most probably simply not appear in the edit form anymore. But it will be displayed when viewing the node. We should also test if the not-exposed summary value gets lost when submitting the edit form, or whether it is magically retained under the hood. :)
#59
The patch has been tested manually. It is working fine, as expcted. There is no "Edit Summary" on the Basic Page content type.
Furtermore, on changing the widget to the summary widget, and entering some nodes with a summary filled in, and then changing it back to the widget without a summary, the previously entered summary value didn't appear in the edit form anymore. Also, the not-exposed summary value was retained.
#60
Tentatively marking RTBC based on #59.
#61
Agreed this could use review from yched or swentel. I can see the use case but I'm ambivalent on fixing it like this. Bumping back to CNR for a bit.
#62
Showing the summary or not rather looks like a 'display_summary' widget setting for the 'textarea_with_summary' widget to me. As noted by @David_Rothstein a long time ago (#7), such a settings currently exists at the instance level, but it's only used by the 'textarea_with_summary' widget.
So ultimately I think the cleanest plan would be to simply merge both 'textarea' and 'texterea_with_summary' widget into a single one, that works on both 'text_long' and 'text_long_with_summary' field types, and with a 'display_summary' setting (hidden when it's used on a 'text_long' field).
Then, the extra param in node_add_body_field() becomes a boolean for the value of the 'display_summary' widget setting.
Thoughts ?
#63
I agree and this is very similar to #1704864: Add a "Required" and "Show by default" option for "Text area with a summary" widget.