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.
Task
Convert the following theme functions to use the new table #type:
Theme function name | Where in Code | What is it really? | Fixed in |
---|---|---|---|
theme_language_content_settings_table | function | form table | this issue |
|
|
|
#1877338: Convert language admin form to new #type 'table' |
|
function | form table | #2422481: Convert language negotiation theme table to table #type |
|
|
|
#1898422: language.module - Convert theme_ functions to Twig |
Remaining tasks
Remove theme functionRemove theme preprocess function- Fix the UI on this page: admin/config/regional/content-language.
- Review patch
- Manual testing
User interface changes
prior to this patch the tables are hidden until the checkbox is clicked. But with the patch, they are all visible initially. UI: admin/config/regional/content-language
Manual testing steps
- Install Drupal 8.
- Enable the language module
- Visit the Content language page at admin/config/regional/content-language
- Checking the boxes under the " Custom language settings" heading will reveal the elements using
'#type' => 'table'
Related issues
Comment | File | Size | Author |
---|---|---|---|
#71 | convert_language-1938912-71.patch | 3.52 KB | joelpittet |
#71 | interdiff.txt | 1.21 KB | joelpittet |
#69 | convert_language-1938912-69.patch | 3.56 KB | joelpittet |
#69 | interdiff.txt | 1.99 KB | joelpittet |
#65 | convert_language-1938912-65.patch | 3.2 KB | joelpittet |
Comments
Comment #1
duellj CreditAttribution: duellj commentedFirst pass at converting the three language tables to table #types
Comment #3
duellj CreditAttribution: duellj commentedThis patch fixes most of the failing tests. There's still one problem though:
_translation_entity_form_language_content_settings_form_alter() alters the language_content_form to inject columns into the table. Should probably wait until #1876718: Allow modules to inject columns into tables more easily is figured out before fixing.
Comment #4
star-szr@duellj - I think we're missing a patch upload here for #3, if you still have it please upload :)
Comment #5
duellj CreditAttribution: duellj commentedWhoops, thanks Cottser! Here's the patch I intended to upload. Let's see if it still applies or needs a reroll
Comment #7
jibranTagging.
Comment #8
jibranTagging.
Comment #9
pplantinga CreditAttribution: pplantinga commentedIt looked like there were quite a few unrelated changes in the last patch, so I just rolled my own.
Comment #11
pplantinga CreditAttribution: pplantinga commentedum. This is obviously more complicated than I thought it was, so someone else should take a look, and probably ignore my patch.
Comment #12
jibran#9: language-tables-1938912-9.patch queued for re-testing.
Comment #13.0
(not verified) CreditAttribution: commentedAdd related Twig conversion
Comment #13.1
Hydra CreditAttribution: Hydra commentedStrike theme_language_admin_overview_form_table, since it has become an _entity_list: 'language_entity'
Comment #14
joelpittetThis should do the trick.
Comment #16
joelpittetOk I think we need sun's help for this one... It's a bit of inception going on here with the form alter manipulating the table. And I'm not sure if I dealt with the keys right. Needs content_translation's theme functions removed and merged in some how.
Comment #17
akalata CreditAttribution: akalata commentedComment #18
akalata CreditAttribution: akalata commentedComment #19
akalata CreditAttribution: akalata commentedComment #20
lokapujyaConversion of just the content language settings page. To simplify the conversion, moving the other table to a new issue.
To simplify review, purposely not making any changes that would require a change to the tests.
Todo:
Remove duplicated "label" field.
Add table dragging and other jQuery enhancements.
Make changes that might require modifications to the test.
Comment #21
lokapujyaComment #22
lokapujyaComment #23
lokapujyaComment #24
lokapujyaCreated the label field like this, to get the heading.
but, then it gets repeated here, in order to satisfy existing tests.
Comment #25
akalata CreditAttribution: akalata commentedComment #26
rpayanmComment #27
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer commentedMinor change for arrays to short syntax.
Comment #29
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer commentedOops! - here's the proper patch... the interdiff on #27 is good.
Comment #33
lokapujyaDid anyone look into the 1 failing test at all?
Comment #34
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer commented@lokapujya looks like the faling test is from a change in core since the patch on #26 was created.
Comment #35
lokapujyaLooks like the part after the
<h4>
is no longer getting included. Not sure why yet.Comment #36
joelpittetOne fail left that's encouraging!
This won't help that but please in the next version consider this:
Same here. +1 for smaller patch!
As much as I love short syntax we should avoid making these changes outside the hunks because if this gets committed it could make another patch need an unnsesasary reroll or visa versa this patch may need a re-roll if this hunk changes in another patch.
Comment #37
lokapujyaActually, it's not the part after the h4, it's part of the title (inside the h4) that is missing.
This is where the text that is missing gets added:
in content_translation.admin.inc, inside _content_translation_form_language_content_settings_form_alter()
It modifies the #title, so got to make that work with the table.
Comment #38
joelpittetSafeMarkup::format() should help that out eh?
Comment #39
lokapujyaYes, could be related to SafeMarkup.
Also, prior to this patch the tables are hidden until the checkbox is clicked. But with the patch, they are all visible initially. I'm just adding ideas that might help incase anyone else is looking at this. Otherwise, I might have time to dig into it in a few days.
Comment #40
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer commentedTaking care of #36 first.
Comment #43
lokapujyaStill needs work, but this fixes the failing test.
Comment #44
lokapujyaThese attributes are already set earlier in the function, need to remove the existing code.
Comment #45
lokapujyaComment #46
lokapujyaComment #47
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer commentedHere's a go at #44, moved the reminding stuff into the new table type form element.
UI is still broken although we are now pulling in the states. I realize the markup is different now as the
h4
label on top is not within a wrappingdiv
as before (which is where the states got attached before).Comment #48
akalata CreditAttribution: akalata commentedComment #49
akalata CreditAttribution: akalata commentedComment #50
akalata CreditAttribution: akalata commentedPosting a reroll.
Comment #51
akalata CreditAttribution: akalata commentedWon't we need that wrapper around the
<h4>
and table? Even if we could get the #states working, they're applied to the table -- which means that all of the<h4>
s would show up, even if their trigger is not checked.Comment #52
star-szrThis might get us a bit closer, the states seem to work now.
Comment #57
lokapujyaNot sure about this, but it might help see the problem.
Comment #60
joelpittetI was playing around with the label at DrupalCon and it would be nice to just extend table.html.twig to add a title/label on top with an h4 wrapping it. That seemed the cleanest approach otherwise you are throwing around render array for some simple text to wrap in an h4 which gets messy fast.
#title property on the #type=>table would be nice. But then I ran into those silly suggestion things not working as I'd expect and having to do the stuff we do with blocks.
Comment #61
RainbowArrayReroll.
Comment #64
joelpittetComment #65
joelpittetI think we may be doing this too complicated of a conversion. Here's the simplest fix-up to a template.
It can be refactored further I'm sure but this at least removes the theme function.
Comment #66
star-szrI really really like that idea, thanks @joelpittet! Just some docs/TX thoughts below:
Per our standards these should be more like "Prepares variables for language content settings table templates." with the default template line per https://www.drupal.org/node/1354#themepreprocess.
Should this point to template_preprocess_language_content_settings_table() instead?
Maybe we can add another variable in preprocess called 'table' that is the same as 'build' (so we're not breaking BC) and print that instead? Build is a weird var name.
Comment #67
joelpittetThe variable was previously named build, but I prefer 'table' too.
Comment #68
star-szrYeah :) so we shouldn't change it because BC but that doesn't stop us from doing…
$variables['table'] = $variables['build'];
…in preprocess.Comment #69
joelpittetHow does this play?
Comment #71
joelpittetThere is some inappropriate stuff happening to 'build' in _content_translation_preprocess_language_content_settings_table().
I'd rather not pass around two big arrays just for the sake of a better named variable. If this wasn't RC I'd say we just change it in all the places.
Comment #72
star-szrRight I didn't think of that. Well thanks for trying :)
Comment #73
lauriiiTested the markup manually and it looks good. Also @Cottsers nitpicks has been fixed and I can't find any new ones.
Comment #75
joelpittetNo harm in trying, thanks for reviewing you two!
Comment #76
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer commentedI like it @joelpittet - much simpler and to the point this patch now. +1 on RTBC
Comment #77
star-szrComment #78
xjmDiscussed with all the committers and we agreed this is an RC target!
Comment #79
webchickCommitted and pushed to 8.0.x. Thanks!