Problem/Motivation
Follow-up to #1498662: Refactor comment entity properties to multilingual
Currently subject field is not configurable via field_ui module and uses custom code to operate.
Node entity already does this and contact module goes too #1856562: Convert "Subject" and "Message" into Message base fields - nice example
Proposed resolution
Use "string" widget for comment.subject field
Remove comment field instance setting to show comment subject in comment form
Remaining tasks
fix tests
User interface changes
Comment subject field widget could be configured for via field_ui interface.
API changes
Comment field instance settings is removed in favor of entity display setting for widget
Comment | File | Size | Author |
---|---|---|---|
#35 | 2292821-comment-subj-widget-35.patch | 56.28 KB | andypost |
Comments
Comment #1
andypostis there any tests for that?
Comment #3
andypostProper patch!
ps; previous was for #2292815: Remove join comments on users table
Comment #4
andypostComment #5
andypostComment #7
andypostShould fix most failures.
Still need to fix comment subject visibility, that stored in field settings... probably better to move it to comment type entity.
Comment #8
andyposta bit more fixed tests...
Now I think that better to remove subject setting from instance and use field_ui to hide that field
Comment #11
andypostFinally polished the patch.
Now
Allow comment title
setting removed in favor of comment type form displayComment #13
andypostFix broken test (none-node)
Comment #14
andypostseems migrate tests are wrong or does not properly check "subject"
Comment #15
andypostStub on migrate - no field instance subject setting migrated.
I found no entity form display migration for comments ;(
Comment #16
andypostSo here is a migrate implementation!
Depending on subject field visibility migrated 1 or 2 comment types:
comment
andcomment_no_subject
Accordingly created 1 or 2 comment fields with instances
Also I found a bug in
ComponentEntityDisplayBase.php
after component is removed no save operation called.So I added a test and fixed existing.
@larowlan if you agree on that approach I will fix comment migration by separating comments within 2 fields
Comment #17
benjy CreditAttribution: benjy commentedLooks ok to me, lets just add the new migration to MigrateDrupal6Test.
Also a few nitpicks I noticed on the way through.
this could be on one line.
bad indent.
Comment #19
andypostFix for migrate
Comment #20
andypostProbably it makes sense to split the issue in #15 and migrate to separate issue
Comment #22
andypostFix broken tests, now migration needs node table
Comment #23
Gábor HojtsyAdd D8MI topic tag.
Comment #24
yched CreditAttribution: yched commentedUnable to speak about the migrate part, which is above my head atm, but the rest looks fine :-)
Only nitpick :
Shoould be one line ?
Comment #25
andypostre-roll and fix #24
Comment #26
andypostAnother approach here is to use different entity form displays in migration
Comment #27
larowlanWow, so little code to make the change, so much test and migrate code.
Great work.
Re-rolled for #2293773: Field allowed values use dots in key names - not allowed in config
RTBC if re-roll comes back green.
Screenshots from manual testing
Comment #29
andypostProper merge
Comment #30
andypostback to rtbc
Comment #31
larowlan+1 rtbc
Comment #32
alexpottComment #33
andypostre-roll, the comment subject is translatable now
Comment #34
andypostComment #35
andypostremoved
getInfo()
from new tests after #697760: Replace getInfo() in tests with native phpDoc + annotations (following PHPUnit)Comment #37
alexpottWe're changing the UI and the API in this patch :)
Comment #38
andypostFixed issue summary, will file cr later
Comment #39
andypostPatch still applies, filed change record https://www.drupal.org/node/2302379
PS: the issue should be linked to https://www.drupal.org/node/2186583
Comment #40
alexpottCommitted b7959a1 and pushed to 8.x. Thanks!