Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Remove theme_fieldset, create system_preprocess_fieldset
Comment | File | Size | Author |
---|---|---|---|
#26 | convert_fieldset_template-1779152-26.patch | 572 bytes | tedbow |
#24 | convert_fieldset_template-1779152-24.patch | 894 bytes | tedbow |
#21 | convert_fieldset_template-1779152-20.patch | 946 bytes | tedbow |
#19 | convert_fieldset_template-1779152-19.patch | 624 bytes | tedbow |
#17 | convert_fieldset_template-1779152-15.patch | 580 bytes | tedbow |
Comments
Comment #1
vlad.dancerAdded tag
Comment #2
vlad.dancerComment #3
podaroklooks really nice
this is a very good example for feature development, tagging
thanks
Comment #4
podarokafter #1779136: Weekly - Merge Twig sandbox with latest HEAD
need reroll
Comment #5
podarokcommited #2 to 8.x branch
Comment #6
tedbowI have just started working on the twig_engine branch and I think this is causing a problem with the installer.
Basically none of the fieldsets in "install_configure_form" are displayed. This is first form in the installer that actually uses a fieldset.
I have done some step debugging and here is what I have found.
In the theme function around line #1021
$info['function'] at this point equals 'theme_fieldset'
but "function_exists($info['function'])" is FALSE.
So the template is never called and the $output is empty.
I assume this is a problem that only happens in the install but since I am not able to install I can't tell.
Comment #7
podarokremove sprint tag
Comment #8
podarokdid revert #2 http://drupalcode.org/sandbox/pixelmord/1750250.git/commit/11ef077
need work #1777976-14: Installer broken
Comment #9
jenlamptonA few comments
1) This patch includes the removal of a theme function. Please do not do that. Drupal will break if all the theme functions are removed since we only have one theme that uses Twig.
2) There is a new line after the docblock and the opening HTML tag. This will print a new line in your source code. We have standardized against adding newlines after the docblock.
3) Please watch indentation for HTML. (remove extra spaces before the tag)
4) Please watch indentation for twig comments (add spaces aftter Twig logic.)
5) All calls to drupal_attributes should be removed from the templates. Attributes are printed as
{{ attributes }}
Please preprocess as necessary.6) The template did not actually work as-is.
I've taken care of the 6 items above and committed a working version of the template to the front-end branch. Patch attached for your review.
Comment #10
jenlamptonjust a note that I also removed the dpm! duh :)
Comment #11
podarok#9 looks nice for me, thanks @jenlampton for fixes
Comment #12
vlad.dancerThx @jenlampton too!
Comment #14
jenlamptonThis template is missing from the front-end branch, can someone please reapply the patch?
Comment #15
tedbowOk I will do it.
Comment #16
Todd Zebert CreditAttribution: Todd Zebert commentedhere's a new patch but I couldn't test because of some MAMP Segmentation fault when installing the sandbox :(
Thanks @jenlampton
Comment #17
tedbowOk, it seems all of the changes are already in this branch except 1.
I am assuming the file name should now be "fieldset.html.twig"
For "core/includes/form.inc"
All of the changes are already there. In addition there were 2 other changes to "template_preprocess_fieldset"
to make sure '#description' and '#title' are actually keys in the $element array.
Comment #18
vlad.dancerWhy patch from #16 remove theme_fieldset?
Why we need another one _preprocess_fieldset() in system.module if it's already in form.inc like in patch #9?
should be
{% if title is defined %}
Comment #19
tedbow@vlad.dancer
I can't speak to #16. I think @Todd Zebert working on this at the same time but didn't know. But it looks like #16 didn't follow instructions in #9.
I have fixed this in the new patch.
Comment #20
podarokwe should use
- variables:
in twig templates, not$variables:
all other looks good
Comment #21
tedbowFixed.
Comment #22
vlad.dancerLooks good for me.
Comment #23
podarokwhy did You make this ?
Is any reason for?
Comment #24
tedbowSorry, I think I was trying to adhere to the patch in #9.
I am not very knowledgable in twig.
......
After looking at other *.html.twig files it seems I shouldn't have made this change.
Fixed patch.
But we should probably be aware there is discussion about the best pattern for this #1808254: Standardize and simplify the attribute syntax in Twig template files
If might need a change based on that.
Comment #25
podarokplease, update Your code to latest version
Comment #26
tedbowOk update patch. It looks like it is 1 line patch now.
Comment #27
vlad.dancerIt's ok to me
Comment #28
Todd Zebert CreditAttribution: Todd Zebert commentedJen was leading a Twig sprint at Drupal Design Camp LA is how I was involved. All I had was an hour exposure to what we're trying to accomplish here - I wish I could have been more helpful.
Comment #29
podarok#26 looks good
#27 thanks for the review
commited/push to front-end
Thanks to all!!!