Updated: Comment #N

Problem/Motivation

The node_type->has_title property has been added as a hidden option to allow to hide the node title form element.

There is no need for it anymore, we can just directly change the form display.

Proposed resolution

Search has_title, remove all usages.

Remaining tasks

User interface changes

API changes

Contributor tasks needed
Task Novice task? Contributor instructions Complete?
Review patch to ensure that it fixes the issue, stays within scope, is properly documented, and follows coding standards Instructions
CommentFileSizeAuthor
#31 remove-has-title-2229163-31.patch9.77 KBthechanceg
#29 remove-2229163-29.patch17.9 KBrixter
#29 interdiff-23-29.txt11.02 KBrixter
#23 remove_node_type-2229163-23.patch7.04 KBbburg
#15 interdiff.txt974 bytesMichael Hodge Jr
#13 remove_node_type-2229163-13.patch7.01 KBMichael Hodge Jr
#2 remove_node_type-2229163-2.patch7.96 KBMichael Hodge Jr

Comments

Michael Hodge Jr’s picture

Michael Hodge Jr commented
Assigned: Unassigned » Michael Hodge Jr
Michael Hodge Jr’s picture

Michael Hodge Jr commented
Status: Active » Needs review
StatusFileSize
new remove_node_type-2229163-2.patch7.96 KB

see attached

Michael Hodge Jr’s picture

Michael Hodge Jr commented
Assigned: Michael Hodge Jr » Unassigned
tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Needs review » Reviewed & tested by the community

Looks good, thanks!

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Issue tags: +Needs change record

Yep, looks great. This probably needs an issue summary or at least a mention of has_title somewhere in an issue that is about widgets/formatters for base fields.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented

Has someone actually tested that hiding this using the form display works? As title is a required property we would need some way to set a default value. As far as I know this will only be possible with #2226493: Apply formatters and widgets to Node base fields.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

That issue is part of a meta issue which has "(except node.title)" in its issue title, because that was already ported as it was used to implement the functionality :)

So yes, this works, it works even better than it should, in fact. This feature was never visible in the UI in core. But now you can hide the title and create nodes that don't have a title. There's an issue for that, see #2111443: Show a warning when configuring form displays when a field is hidden and has no default value. (issue title is not correct, ignore that ;))

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented

Duh, sorry. Yes, that makes sense! Thanks for the explanation.

jessebeach’s picture

jessebeach commented

Here is the draft change record. We need to have it filled out before committing this issue: https://drupal.org/node/2231763

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Issue tags: -Needs change record

Updated the change record, should be ready to go.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/modules/migrate_drupal/lib/Drupal/migrate_drupal/Tests/Dump/Drupal6DumpBase.php
@@ -80,12 +80,6 @@ protected function tableDefinitions() {
-          'has_title' => array(
-            'description' => 'Boolean indicating whether this type uses the {node}.title field.',
-            'type' => 'int',
-            'unsigned' => TRUE,
-            'not null' => TRUE,
-            'size' => 'tiny'),

This is the Drupal 6 database. We shouldn't be removing this.

Michael Hodge Jr’s picture

Michael Hodge Jr commented
Assigned: Unassigned » Michael Hodge Jr

I'll fix it and get another patch submitted.

Michael Hodge Jr’s picture

Michael Hodge Jr commented
Status: Needs work » Needs review
StatusFileSize
new remove_node_type-2229163-13.patch7.01 KB

I've removed the overreaching aspect of the patch, and re-applied it/tested against the latest version in HEAD.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Good point, but what should we be doing with it then? I guess we need to check if there is a already a test for it in the IMP sandbox? https://drupal.org/sandbox/chx/2105305

Not sure I care enough to add a specific migration for it and a test, if there isn't one yet? My guess is that people first need a module to take care of a default value for the title before it is useful to hide it again, see #2111443: Show a warning when configuring form displays when a field is hidden and has no default value.

@Michael Hodge Jr: When updating a patch, it is helpful to provide an interdiff to make it easier to see the difference.

Michael Hodge Jr’s picture

Michael Hodge Jr commented
StatusFileSize
new interdiff.txt974 bytes

Thanks @Berdir. Attached is the interdiff.txt

Michael Hodge Jr’s picture

Michael Hodge Jr commented
Assigned: Michael Hodge Jr » Unassigned
berdir’s picture

berdir
Primary language German
Location Switzerland
commented

13: remove_node_type-2229163-13.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 13: remove_node_type-2229163-13.patch, failed testing.

Michael Hodge Jr’s picture

Michael Hodge Jr commented
Assigned: Unassigned » Michael Hodge Jr

It looks like the patch may need to be re-rolled. I can work on that and get another patch up and ready for testing.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

The migrate patch landed, I assume it conflicted with that someone.

As discussed with the migrate time, if there is a migration defined for has_title somewhere, then we should remove it and open a minor follow-up issue to move it to the entity form display. As discussed above, I'm not sure if we should really do that but we can discuss that there.

cilefen’s picture

cilefen commented
Issue summary: View changes

Getting ready for the Austin DrupalCon sprint, following http://www.hook42.com/blog/prepping-drupalcon-austin-sprints-sprint-lead...

bburg’s picture

bburg
he/him
Location Washington, DC
commented
Assigned: Michael Hodge Jr » bburg

Working on this patch re-roll at DrupalCon Austin.

bburg’s picture

bburg
he/him
Location Washington, DC
commented
Status: Needs work » Needs review
StatusFileSize
new remove_node_type-2229163-23.patch7.04 KB

Re-rolled patch in 13.

bburg’s picture

bburg
he/him
Location Washington, DC
commented
Assigned: bburg » Unassigned

Releasing this from myself as I am unable to work on this for the moment. Maybe some folks at the DrupalCon sprint are interested.

rixter’s picture

rixter commented

I'm at the sprint & working on it…

rixter’s picture

rixter commented
Assigned: Unassigned » rixter
rixter’s picture

rixter commented
Assigned: rixter » Unassigned

Unassigned, but still working on this...

rixter’s picture

rixter commented
Issue summary: View changes
rixter’s picture

rixter commented
StatusFileSize
new interdiff-23-29.txt11.02 KB
new remove-2229163-29.patch17.9 KB

Removed all incidences on has_title, including in the migrations, based on comment #20.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Needs work

We shouldn't remove it from the source, because it does exist there, and as discussed above, we might want to consider migrating it to a component configuration later. So remove it from the tests and migration files but not the D6 dumps.

thechanceg’s picture

thechanceg commented
Status: Needs work » Needs review
StatusFileSize
new remove-has-title-2229163-31.patch9.77 KB

I rerolled that patch with the suggestion of Berdir in #30.

Status: Needs review » Needs work

The last submitted patch, 31: remove-has-title-2229163-31.patch, failed testing.

thechanceg’s picture

thechanceg commented
Status: Needs work » Needs review

31: remove-has-title-2229163-31.patch queued for re-testing.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Reviewed & tested by the community

Looks good, thanks.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed 3154700 and pushed to 8.x. Thanks!

  • Commit c723131 on 8.x by alexpott: 
    Issue #2229163 by Michael Hodge Jr, rixter, thechanceg, bburg | Berdir:...
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

Committed c723131 and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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