The function bootstrap_preprocess_bootstrap_panel() is responsible for generating the variables used in the template bootstrap-panel.tpl.php.

Line 35 of bootstrap-panel.vars.php sets the ID attribute of the element that wraps the panel:

<?php

  if (isset($element['#id'])) {
    $attributes['id'] = $element['#id'];
    $variables['target'] = '#' . $element['#id'] . ' > .collapse';
  }

?>

However, this does not respect an ID that is set on the element elsewhere in the code.

For example, if I create a new Form API form fieldset, and set a custom ID on it:

<?php

$form['myfieldset'] = array(
  '#type' => 'fieldset',
  '#title' => t('My Fieldset'),
  '#attributes' => array(
    'id' => 'my-specific-fieldset-id',
  ),
);

?>

Normally, that ID 'my-specific-fieldset-id' would be set on the fieldset element in the final HTML. With the Bootstrap theme, however, it is overridden.

It should be: #my-specific-fieldset-id
Instead, it is: #edit-myfieldset

CommentFileSizeAuthor
#7 bootstrap_panel_element_id-2468625-7.patch2.27 KBm.stenta
#5 bootstrap_panel_element_id-2468625-5.patch2.66 KBm.stenta
#1 bootstrap_panel_element_id-2468625-1.patch1.13 KBm.stenta

Comments

m.stenta’s picture

m.stenta
he/him
Primary language English
commented
Status: Active » Needs review
StatusFileSize
new bootstrap_panel_element_id-2468625-1.patch1.13 KB

Attached is a patch that fixes this issue. The approach I took was to check to see if $attributes['id'] is already set (it will be if $element['#attributes']['id'] is), and use that instead of $element['#id'] if it is. But it also uses $element['#id'] as a fallback if it's available (not sure if there's a specific reason that is being used). So it works in both cases.

asrob’s picture

asrob
he/him
Primary language Hungarian
Location Hungary 🇭🇺 🇪🇺
commented
Status: Needs review » Reviewed & tested by the community

I tested it and it looks good to me, that's why I change its status to RTBC.

markhalliwell’s picture

markhalliwell
Primary language English
commented
Status: Reviewed & tested by the community » Needs work

This really should be using element_set_attributes() instead.

m.stenta’s picture

m.stenta
he/him
Primary language English
commented

@markcarver: Thanks for the input. I will try to put together a new patch soon.

m.stenta’s picture

m.stenta
he/him
Primary language English
commented
Status: Needs work » Needs review
StatusFileSize
new bootstrap_panel_element_id-2468625-5.patch2.66 KB

Attached is a new patch which reworks the bootstrap_preprocess_bootstrap_panel() function to use element_set_attributes().

I tried to follow the same approach taken in bootstrap_preprocess_button(). I added some comments to make the logic easier to follow, and tested to make sure that collapsible panels still work.

markhalliwell’s picture

markhalliwell
Primary language English
commented
Status: Needs review » Needs work 
  1. +++ b/templates/bootstrap/bootstrap-panel.vars.php
@@ -9,11 +9,18 @@
-  $attributes = !empty($element['#attributes']) ? $element['#attributes'] : array();
-  $attributes['class'][] = 'panel';
...
+  $element['#attributes']['class'][] = 'panel';

    I wasn't really suggesting the variable be changed. That being said... if we're really going to refactor this, it should really be using _bootstrap_get_attributes():

    // Set the ID attribute of the element.
element_set_attributes($element, array('id'));

// Retrieve the attributes for the element.
$attributes = &_bootstrap_get_attributes($element);
$attributes['class'][] = 'panel';

  2. +++ b/templates/bootstrap/bootstrap-panel.vars.php
@@ -9,11 +9,18 @@
+  element_set_attributes($element, array('id', 'name', 'value', 'type'));

    Only need id here. 

  3. +++ b/templates/bootstrap/bootstrap-panel.vars.php
@@ -9,11 +9,18 @@
+  // Add panel and panel-default classes.

    This is a rather redundant comment and isn't needed. 

  4. +++ b/templates/bootstrap/bootstrap-panel.vars.php
@@ -50,12 +61,12 @@ function bootstrap_preprocess_bootstrap_panel(&$variables) {
-  $variables['attributes'] = $attributes;
...
-  $variables['attributes'] = drupal_attributes($variables['attributes']);
+  $element = &$variables['element'];
+  $variables['attributes'] = drupal_attributes($element['#attributes']);

    I know this should be like this, but IIRC I had created this way specifically so the theme hook could be invoked directly (without an element being passed via FAPI). I could be mistaken and it may actually work with these changes if _bootstrap_get_attributes() is implemented.

m.stenta’s picture

m.stenta
he/him
Primary language English
commented
StatusFileSize
new bootstrap_panel_element_id-2468625-7.patch2.27 KB

Updated patch to use _bootstrap_get_attributes() per comment #6.1.
Removed all attributes except for id (per #6.2).
Reverted code in bootstrap_process_bootstrap_panel() (per #6.4)

I left the comment in that you mentioned in #6.3. I agree it's not really necessary, but felt that it was better to leave it for consistency with the structure of other comments and code. I like to err on the side of over-commenting, but it's not that important, so I can remove that too if you want.

m.stenta’s picture

m.stenta
he/him
Primary language English
commented
Status: Needs work » Needs review

  • markcarver committed 01090de on 7.x-3.x authored by m.stenta 
    Issue #2468625 by m.stenta: bootstrap_preprocess_bootstrap_panel()...
markhalliwell’s picture

markhalliwell
Primary language English
commented
Version: 7.x-3.x-dev » 8.x-3.x-dev
Status: Needs review » Patch (to be ported)

Tried cherry picking to 8.x-3.x, but that function has already undergone a significant amount of work, a new patch will need to be created from #7.

markhalliwell’s picture

markhalliwell
Primary language English
commented
Version: 8.x-3.x-dev » 7.x-3.x-dev
Status: Patch (to be ported) » Closed (fixed)
Related issues: +#2632718: Refactor bootstrap_panel

Closing this since 8.x is much more complicated and requires it's own focus.