Problem/Motivation

Because of the recent commit from #2595613: '#title_display' => 'invisible' does not work for composite form elements, it causes a "PHP Notice: Undefined index: #title_display in /usr/local/var/www/d8.dev/www/core/includes/form.inc on line 209".

To reproduce it: run the following command (fit it with your dev instance)
php ~/www/d8.dev/www/core/scripts/run-tests.sh --url http://d8.dev --browser --verbose --color --concurrency 4 --class 'Drupal\taxonomy\Tests\TermIndexTest'

Proposed resolution

I'll upload a patch adding an isset() check to fix it.

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#8 php_notice-2665202-8.patch550 bytestduong
#8 interdiff-2665202-2-8.txt501 bytestduong
#2 php_notice-2665202-2.patch586 bytestduong

Comments

tduong created an issue. See original summary.

tduong’s picture

tduong commented
Component: php.module » forms system
Assigned: Unassigned » tduong
StatusFileSize
new php_notice-2665202-2.patch586 bytes

Uploaded patch adding isset() in template_preprocess_fieldset() (form.inc).

edurenye’s picture

edurenye commented
Status: Active » Needs review

Looks good to me.

tduong’s picture

tduong commented
Issue summary: View changes
swentel’s picture

swentel commented

Darn, should we add a test for it just to be sure ?

swentel’s picture

swentel commented

Just wondering: on line 198, there's a already a check on #title_display and assigns it to $variables['title_display'] so maybe we can change the patch just to

  if ($variables['title_display'] == 'invisible') {
swentel’s picture

swentel commented
Status: Needs review » Needs work

Setting to needs work for #6

tduong’s picture

tduong commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-2665202-2-8.txt501 bytes
new php_notice-2665202-8.patch550 bytes

Rerolled and edited the patch according to #6. About the test, it could be nice but I'm not sure how it should be like.

swentel’s picture

swentel commented

Hmm, yeah I tried creating a test too, but I can't trigger it at all - I thought just creating a form element without the #title_display property would do it, but it doesn't trigger the notice at all so I'm wondering from which form it comes from when you do the run-tests ?

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

It somehow happens with an element that has '#theme' => 'simpletest_result_summary'. We didn't check yet which one exactly, why it only happens in run-tests.sh and not the UI and why that is even calling that fieldset preprocess function...

Note that it only happens when you use --browser.

tduong’s picture

tduong commented
Issue summary: View changes
swentel’s picture

swentel commented

It happens in SimpletestResultsForm::addResultForm - funky :)

Don't know if we can even test it, so RTBC for me.

swentel’s picture

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

Woops, forgot to move the status.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented

This is failing my contrib tests now.
Shouldn't we be adding test coverage? Or reverting the original patch?

swentel’s picture

swentel commented

@tim.plunkett does it fail your contrib tests on the bot or in the UI ? Because I think it only happens if you use --browser.

SimpletestResultsForm::addResultForm has this code, note the comment

    $form['result'] = array(
      '#type' => 'fieldset',
      '#title' => 'Results',
      // Because this is used in a theme-less situation need to provide a
      // default.
      '#attributes' => array(),
    );

I don't think it has a test for that behavior of #attributes either.

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Reviewed & tested by the community » Needs review

I reverted #2595613: '#title_display' => 'invisible' does not work for composite form elements; can we fix this in that issue and mark this as a duplicate, or is there a reason to keep a separate issue?

swentel’s picture

swentel commented
Status: Needs review » Closed (duplicate)

We'll fix it over there.