Problem/Motivation

The code that adds the default values to the exposed filters in linkchecker.module cause an 'Undefined index' error when the filters are deleted.

Steps to reproduce

Edit the "Broken Links Report" view and delete the "Result" and "Status code" filters.

Proposed resolution

Add a couple checks to the alter hook to make sure the filters exist before adding the new options. There may be a more elegant way of doing this, but I needed a patch quick.

Remaining tasks

Review, maybe come up with a better solution?

CommentFileSizeAuthor
#14 3186322-test-only.patch5.25 KBeiriksm
#12 3186322-test-only.patch3.23 KBeiriksm
#10 3186322-test-only.patch2.76 KBeiriksm
#5 3186322-test-only.patch2.64 KBeiriksm

Issue fork linkchecker-3186322

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

xaqrox created an issue. See original summary.

xaqrox opened merge request !1

eiriksm’s picture

eiriksm
he/him
Primary language Norwegian Bokmål
Location Norway
CreditAttribution: eiriksm at Ny Media AS, Violinist, Foreningen Drupal Norge commented
Status: Active » Reviewed & tested by the community

Had the same problem.

I don't see what the states part is trying to achieve, so I was planning on opening a similar issue, but this works!

eiriksm’s picture

eiriksm
he/him
Primary language Norwegian Bokmål
Location Norway
CreditAttribution: eiriksm at Ny Media AS, Violinist, Foreningen Drupal Norge commented
Status: Reviewed & tested by the community » Needs work

I am going to write a quick test for this

eiriksm’s picture

eiriksm
he/him
Primary language Norwegian Bokmål
Location Norway
CreditAttribution: eiriksm at Ny Media AS, Violinist, Foreningen Drupal Norge commented
Status: Needs work » Needs review
FileSize
3186322-test-only.patch2.64 KB

Added a test in the fork, and here is test only patch that should be failing.

Status: Needs review » Needs work

The last submitted patch, 5: 3186322-test-only.patch, failed testing. View results

eiriksm’s picture

eiriksm
he/him
Primary language Norwegian Bokmål
Location Norway
CreditAttribution: eiriksm at Ny Media AS, Violinist, Foreningen Drupal Norge commented
Status: Needs work » Needs review
JeroenT’s picture

JeroenT
Primary language Dutch
Location 🇧🇪
CreditAttribution: JeroenT at iO commented

The state was added in #3112908: Adjustments to "Broken Links" view.

When you filter on a result (e.g. "Server Error (5xx)") the code field doesn't make any sense. The result filter makes it possible to filter on a group of HTTP status codes while the code field makes it possible to filter on a single http status code.

eiriksm’s picture

eiriksm
he/him
Primary language Norwegian Bokmål
Location Norway
CreditAttribution: eiriksm at Ny Media AS, Violinist, Foreningen Drupal Norge commented
Status: Needs review » Needs work

Ahh, I see that now. Thanks!

I will just update my test to test that as well, so we get coverage for it.

eiriksm’s picture

eiriksm
he/him
Primary language Norwegian Bokmål
Location Norway
CreditAttribution: eiriksm at Ny Media AS, Violinist, Foreningen Drupal Norge commented
Status: Needs work » Needs review
FileSize
3186322-test-only.patch2.76 KB

New test only patch

Status: Needs review » Needs work

The last submitted patch, 10: 3186322-test-only.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

eiriksm’s picture

eiriksm
he/him
Primary language Norwegian Bokmål
Location Norway
CreditAttribution: eiriksm at Ny Media AS, Violinist, Foreningen Drupal Norge commented
Status: Needs work » Needs review
FileSize
3186322-test-only.patch3.23 KB

New test only patch

eiriksm’s picture

eiriksm
he/him
Primary language Norwegian Bokmål
Location Norway
CreditAttribution: eiriksm at Ny Media AS, Violinist, Foreningen Drupal Norge commented
Status: Needs review » Needs work

Ouch. I was fearing that was the case. No php notice fails in webdriver tests. Probably add them both then?

eiriksm’s picture

eiriksm
he/him
Primary language Norwegian Bokmål
Location Norway
CreditAttribution: eiriksm at Ny Media AS, Violinist, Foreningen Drupal Norge commented
Status: Needs work » Needs review
FileSize
3186322-test-only.patch5.25 KB

New test only with both a functional test and functionaljavascript test, that should hopefully fail, while the MR should pass.

Status: Needs review » Needs work

The last submitted patch, 14: 3186322-test-only.patch, failed testing. View results

eiriksm’s picture

eiriksm
he/him
Primary language Norwegian Bokmål
Location Norway
CreditAttribution: eiriksm at Ny Media AS, Violinist, Foreningen Drupal Norge commented
Status: Needs work » Needs review
JeroenT’s picture

JeroenT
Primary language Dutch
Location 🇧🇪
CreditAttribution: JeroenT at iO commented
Status: Needs review » Reviewed & tested by the community

Looks good to me.

eiriksm’s picture

eiriksm
he/him
Primary language Norwegian Bokmål
Location Norway
CreditAttribution: eiriksm at Violinist, Foreningen Drupal Norge, Ny Media AS commented
Status: Reviewed & tested by the community » Fixed

  • eiriksm committed c92948d on 8.x-1.x authored by xaqrox 
    Issue #3186322 by eiriksm, JeroenT: 'All' options in...
eiriksm’s picture

eiriksm
he/him
Primary language Norwegian Bokmål
Location Norway
CreditAttribution: eiriksm at Violinist, Foreningen Drupal Norge, Ny Media AS commented

Thanks! Committed!

Status: Fixed » Closed (fixed)

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