Support from Acquia helps fund testing for Drupal Acquia logo

Comments

vlad.dancer’s picture

Issue tags: +code sprint drupal night ua 2012

Added tag

vlad.dancer’s picture

Status: Active » Needs review
FileSize
3.48 KB
podarok’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +documentation guide, +sprint, +Twig

looks really nice
this is a very good example for feature development, tagging
thanks

podarok’s picture

Status: Reviewed & tested by the community » Needs work

after #1779136: Weekly - Merge Twig sandbox with latest HEAD

git apply --check 1779152-theme-fieldset-2.patch
error: patch failed: core/includes/form.inc:2766
error: core/includes/form.inc: patch does not apply

need reroll

podarok’s picture

Component: Code » Twig templates
Status: Needs work » Fixed

commited #2 to 8.x branch

tedbow’s picture

Component: Twig templates » Twig engine
Category: task » bug
Status: Fixed » Active

I 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

  // Generate the output using either a function or a template.
  $output = '';
  if (isset($info['function'])) {
    if (function_exists($info['function'])) {
      $output = $info['function']($variables);
    }
  }
  else {

$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.

podarok’s picture

Issue tags: -code sprint drupal night ua 2012

remove sprint tag

podarok’s picture

Status: Active » Needs work
Issue tags: +code sprint drupal night ua 2012
jenlampton’s picture

Status: Needs work » Fixed
Issue tags: -sprint, -code sprint drupal night ua 2012
FileSize
2.29 KB

A 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.

jenlampton’s picture

just a note that I also removed the dpm! duh :)

podarok’s picture

#9 looks nice for me, thanks @jenlampton for fixes

vlad.dancer’s picture

Thx @jenlampton too!

Status: Fixed » Closed (fixed)

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

jenlampton’s picture

Category: bug » task
Status: Closed (fixed) » Active

This template is missing from the front-end branch, can someone please reapply the patch?

tedbow’s picture

Assigned: vlad.dancer » tedbow

Ok I will do it.

Todd Zebert’s picture

Assigned: tedbow » vlad.dancer
Status: Active » Needs review
FileSize
3.31 KB

here's a new patch but I couldn't test because of some MAMP Segmentation fault when installing the sandbox :(
Thanks @jenlampton

tedbow’s picture

Ok, 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.

vlad.dancer’s picture

Status: Needs review » Needs work

Why 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?

+++ b/core/themes/stark/templates/form.inc/fieldset.html.twig
@@ -15,7 +15,7 @@
   {% if title %}

should be
{% if title is defined %}

tedbow’s picture

Status: Needs work » Needs review
FileSize
624 bytes

@vlad.dancer

Why 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?

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.

should be
{% if title is defined %}

I have fixed this in the new patch.

podarok’s picture

Status: Needs review » Needs work
Issue tags: +Needs documentation updates
+++ b/core/themes/stark/templates/form.inc/fieldset.twigundefined
@@ -0,0 +1,33 @@
+ * $variables: An associative array containing:

we should use - variables: in twig templates, not $variables:

all other looks good

tedbow’s picture

Status: Needs work » Needs review
FileSize
946 bytes

we should use - variables: in twig templates, not $variables:

Fixed.

vlad.dancer’s picture

Looks good for me.

podarok’s picture

Status: Needs review » Postponed (maintainer needs more info)
+++ b/core/themes/stark/templates/form.inc/fieldset.html.twigundefined
@@ -15,8 +15,8 @@
-<fieldset class="{{ attributes.class }}" {{ attributes }}>
-  {% if title %}
+<fieldset {{ attributes }}>

why did You make this ?
Is any reason for?

tedbow’s picture

Sorry, 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.

podarok’s picture

Status: Postponed (maintainer needs more info) » Needs work
git apply --check convert_fieldset_template-1779152-24.patch
error: patch failed: core/themes/stark/templates/form.inc/fieldset.html.twig:5
error: core/themes/stark/templates/form.inc/fieldset.html.twig: patch does not apply

please, update Your code to latest version

tedbow’s picture

Ok update patch. It looks like it is 1 line patch now.

vlad.dancer’s picture

Status: Needs work » Reviewed & tested by the community

It's ok to me

Todd Zebert’s picture

Jen 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.

podarok’s picture

Status: Reviewed & tested by the community » Fixed

#26 looks good
#27 thanks for the review

commited/push to front-end

Thanks to all!!!

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