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
Almost everything is in the title.
Node allows to change the title label per content type. It's an important feature for UX so we should mimic it in Group.
Proposed resolution
Add a setting in the group type edit form to set the title label.
Remaining tasks
Patch, Review, Commit
User interface changes
A new required form field in the group type edit form: Title field label.
API changes
None.
Data model changes
Store the title label in the group type.
Comment | File | Size | Author |
---|---|---|---|
#16 | group-2774649-16.patch | 5.42 KB | kristiaanvandeneynde |
|
Comments
Comment #2
ozinPatch attached, please check.
Comment #3
imclean CreditAttribution: imclean commentedThis is where I've gone wrong. As with ECK and other custom entities, the first thing I do is remove the title field. Would it be possible to use an entity label rather than title? The title field can be removed in from various entity types, including group entities, and the field seems to be left over from Node days. Being able to select any appropriate field as the entity title would allow more options. E.g. text, list (text), email, date etc.
That said, groups may not generally require that kind of flexibility.
Comment #4
kristiaanvandeneyndeRe #3: Content entities definitely need a label field. You are free to hide it, though.
Patch looks good at first sight, I'd like to add it with a test included. A web test that submits the form and then retrieves the field from the field manager would do. Anyone care to tackle this?
Comment #5
kristiaanvandeneyndeComment #6
ozinI will take care about this.
Comment #7
ozinNew patch attached please check.
Comment #8
imclean CreditAttribution: imclean commented#4,
Config entities can also have a label field. (Sorry, a bit off topic now.)
Comment #9
kristiaanvandeneyndeTest looks good, thanks. Could you rename it to GroupTypeWebTest and write it as a PHPUnit test? I've been trying to avoid adding any Simpletest tests to the module as it seems like it will be unsupported soon(ish).
Comment #10
Taran2LDrupal now uses short array syntax. See coding standards There are more places where long array syntax is being used. Please fix all of them
Probably
!==
can be used hereSimpletest is deprecated. Use
Drupal\Tests\BrowserTestBase
Change record
Comment does not reflect what exactly is being tested
Use {@inheritdoc}
Comment #11
ozin@Taran2L thanks for review, I agree with yours remarks.
@kristiaanvandeneynde are you agree with using BrowserTestBase? We need to test form submit because we have logic in the Form save.
Comment #12
kristiaanvandeneynde@ozin I think BrowserTestBase supports POST, so that should be fine yeah.
Comment #13
ozinAdded test and fixed remarks. Please check.
Comment #14
kristiaanvandeneyndeCode looks good, test needs some love regarding where to put what code. But the overall patch is something I can work with. Thanks!
Comment #15
mparker17The patch in #13 worked for me: boldly marking as RTBC.
Also un-assigning as last activity was "about a year ago".
@kristiaanvandeneynde, I could fix the patch to fix the test, but I'm not sure what needs to be done.
Comment #16
kristiaanvandeneyndeThis is clean enough. No interdiff as the patch is small and I moved a lot of code around.
Comment #18
kristiaanvandeneyndeComment #19
mparker17Awesome, thanks!