Fields within fieldsets are not having the placeholder text set.

CommentFileSizeAuthor
#5 webform_hints-fixes-fieldsets-2070219-3.patch1.79 KBadamzimmermann
#3 webform_hints-fixes-fieldsets-2070219-3.patch1.77 KBguschilds
#2 webform_hints-fixes-fieldsets-2070219-0.patch1.12 KBadamzimmermann
#1 fixes fields within fieldsets-2070219.patch1.13 KBadamzimmermann

Comments

adamzimmermann’s picture

adamzimmermann commented
StatusFileSize
new fixes fields within fieldsets-2070219.patch1.13 KB

There might be a better way to check for fields items that are theme configuration other than just checking for '#', but I couldn't find a Drupal function that seemed to do what I needed.

adamzimmermann’s picture

adamzimmermann commented
StatusFileSize
new webform_hints-fixes-fieldsets-2070219-0.patch1.12 KB

I thought this through some more, and using element_children wouldn't actually be better in my opinion. It would add the overhead of that function, and then require a loop that works with the flipped array keys, which is always annoying. Additionally, we would be calling webform_hints_add_title using a different syntax, introducing more opportunities for error. So I think a quick check at the beginning of the function call is the simplest and probably most efficient solution here.

Thoughts?

I renamed the patch using the correct naming syntax and re-attached it.

guschilds’s picture

guschilds commented
StatusFileSize
new webform_hints-fixes-fieldsets-2070219-3.patch1.77 KB

element_children may bring a sliver of overhead, but it avoids calling webform_hints_add_title on a decent chunk of irrelevant items. You're right, though, the current implementation that uses array_walk makes it hard to use element_children. I've attached a patch that switches that up. This is totally bikeshedding and I only did it because element_children feels more like "the Drupal way" to me.

Even if you disagree, there are a few things worth noting:

  • Your patch introduced isset() into if (isset($element['#required']) && !$element['#required']) {, but if it isn't set that probably means the element isn't required. This statement should be TRUE when it isn't, so using OR makes more sense, right?
  • It seems like the whole point of // Skip the boolean #tree. / if (is_array($element)) { was to do the same thing you're now doing with your check (and I'm doing with element_children), so that is no longer needed. I believe that can just become else, right?

Marking as needs work. Feel free to decide whatever route you want to take, but please consider the two bulleted changes. Thanks!

guschilds’s picture

guschilds commented
Status: Active » Needs work

Hmm. On second thought, ignore the suggested change to the required logic. Didn't think that one through. :)

The is_array check also doesn't hurt, I suppose. It could be moved up with the $key[0] check. If not, please update the comment.

Sorry bout that!

adamzimmermann’s picture

adamzimmermann commented
Status: Needs work » Needs review
StatusFileSize
new webform_hints-fixes-fieldsets-2070219-3.patch1.79 KB

Alright. I think this is the one.

guschilds’s picture

guschilds commented
Status: Needs review » Reviewed & tested by the community

I'd agree. Just reviewed/tested it and it works well. Thanks for this!

adamzimmermann’s picture

adamzimmermann commented
Status: Reviewed & tested by the community » Fixed

Fix committed here.

Status: Fixed » Closed (fixed)

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