Problem

  • When #states is attached to a form element that contains #markup the following notice occurs:

    Notice: Undefined index: #type in drupal_process_states() (line 583 of core/includes/common.inc).

Cause

  • This happens because a #markup element does not require the #type to be set.

Proposed solution

  1. Fix the PHP notice.
  2. Add a regression (kernel) test to ensure that this bug does not reappear in the future.

Notes

  • There are no PHP/backend tests for #states right now, because the "functionality" on the PHP side is in essence just a json_encode(), so nothing worth to test.

Comments

pfrenssen created an issue. See original summary.

zuhair_ak’s picture

Just added an isset test for the #type index in line 580 of core/includes/common.inc. Dont know if this is the correct way.

zuhair_ak’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 2: undefined_index_type_states-2700667-2.patch, failed testing.

zuhair_ak’s picture

Status: Needs work » Needs review
FileSize
727 bytes

There was no newline at the end of patch.Fixed it.

msypes’s picture

Hi,
Michael & Joe are working on this patch at the NoLa 2016 sprint

Checked by attempting a temporary hack with the Account Settings form (admin/config/people/accounts), but couldn't fully reproduce the problem with a markup element. I can confirm that commenting out the #type for the Logo Settings in the Appearance settings form ( /admin/appearance/settings) does break the expected Javascript functionality as well as generate the error in the recent logs.
The patch is straightforward and corrects the described issue.

msypes’s picture

Status: Needs review » Reviewed & tested by the community
pfrenssen’s picture

This looks good to me. This doesn't require a test IMO. RTBC+1, thanks!

esod’s picture

Where can I recreate the bug? /admin/config/people/accounts doesn't print the undefined index notice. There's nothing in the log. Thanks.

alexpott’s picture

Priority: Minor » Normal
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

I definitely think this could use a test - it would be easy to produce a regression.

msypes’s picture

@esod: To be clear, the error doesn't normally occur on the page I referenced (admin/config/people/accounts). I tried modifying that admin form by adding a #markup element whose display would depend on the checking of another form element on the page. I couldn't get such a thing to work, which would have duplicated the originally described issue/problem.
On the other hand, commenting out the #type for the Logo Settings in the Appearance settings form ( /admin/appearance/settings) does break the expected Javascript functionality as well as generate the error in the recent logs.

ankitasharma13’s picture

Status: Needs work » Needs review
Issue tags: +Tested

I have applied the above patch and tested and its working fine.There is nothing in the error log.

dcam’s picture

Status: Needs review » Needs work
Issue tags: -Tested

Thank you for the review, @ankitasharma13!

Unfortunately, this issue still needs work. @alexpott's comment in #10 and the "Needs tests" tag are intended to indicate that this patch should be expanded to include automated tests.

zuhair_ak’s picture

I am new to writing Unit tests, can someone give an example to write tests for core/includes/common.inc functions like here in drupal_process_states function? Is there examples for that in core?

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

mradcliffe’s picture

Issue tags: +Dublin2016

This probably needs a re-roll as well these days.

finn.lewis’s picture

I'm looking at this at the DrupalCon mentored sprints.
I'll run through https://www.drupal.org/contributor-tasks/reroll

finn.lewis’s picture

The patch applies cleanly on 8.3.x.
Not too sure where to start with writing tests, so I'll leave this for someone else.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

ajmantis’s picture

Retest to 8.4

sun’s picture

pk188’s picture

Status: Needs work » Needs review
FileSize
727 bytes

Adding patch to retest #5.