Problem/Motivation

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 resolution

  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.

Remaining tasks

  • Make sure patch applies to 8.5.x and 8.6.x as well.
  • Write an unit (kernel) test to ensure that the bug does not reappear in the future.

User interface changes

No.

API changes

No.

Data model changes

No

Members fund testing for the Drupal project. Drupal Association Learn more

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.

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

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

Version: 8.5.x-dev » 8.6.x-dev

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

Alex Zu’s picture

The patch applies well on 8.4.4. Thank you.

mradcliffe’s picture

Issue summary: View changes
Status: Needs review » Needs work

Updated issue summary to state remaining tasks.

It looks like the patch needs a test before any further review can be done so I am setting this back to Needs Work.