CommentFileSizeAuthor
#12 search_api_page-n2754411-12.patch6.19 KBDamienMcKenna
#12 search_api_page-n2754411-12.interdiff.txt1.93 KBDamienMcKenna
#11 2754411-11.patch5.92 KBErik Frèrejean
#11 2754411-9-11-inderdiff.txt1.15 KBErik Frèrejean
#9 2754411-8-9-interdiff.txt945 bytesErik Frèrejean
#9 2754411-9.patch5.82 KBErik Frèrejean
#8 2754411-8.patch5.79 KBErik Frèrejean
#6 2754411.patch5.49 KBborisson_
#6 2754411-interdiff.txt4.47 KBborisson_
#5 2754411.patch3.11 KBborisson_
#2 search_api_page-2754411-2-cache.patch505 bytesSimon Georges
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

swentel created an issue. See original summary.

Simon Georges’s picture

Simon Georges CreditAttribution: Simon Georges at Makina Corpus commented
Status: Active » Needs review
FileSize
search_api_page-2754411-2-cache.patch505 bytes

Hi,
I'm willing to help, since I currently have the issue on one of my site. It's fine when I'm logged in as admin, but as an anonymous user, the search results don't change when changing search term.
So, I simply added a cache url context to fix it, but I'm willing to go further if you can point me in the right direction.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ commented
Issue tags: +Needs tests

Should we also add the role cache context or is that one added by default? In any case this is a solid improvement but we should probably also test this.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ commented
Status: Needs review » Needs work

NW per #3

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ commented
Status: Needs work » Needs review
FileSize
2754411.patch3.11 KB

I don't see the problem mentioned in #2. I tried to make a test that failed.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ commented
Issue tags: -Needs tests
FileSize
2754411-interdiff.txt4.47 KB
2754411.patch5.49 KB

Status: Needs review » Needs work

The last submitted patch, 6: 2754411.patch, failed testing. View results

Erik Frèrejean’s picture

Erik Frèrejean
Primary language Dutch
Location 🏴‍☠️
CreditAttribution: Erik Frèrejean at Dutch Open Projects, KennisMeester commented
FileSize
2754411-8.patch5.79 KB

Rerolled the patch from #6.

Erik Frèrejean’s picture

Erik Frèrejean
Primary language Dutch
Location 🏴‍☠️
CreditAttribution: Erik Frèrejean at Dutch Open Projects, KennisMeester commented
FileSize
2754411-9.patch5.82 KB
2754411-8-9-interdiff.txt945 bytes

The tests fail because the searched_fields option isn't added in ::setUpPage().

Erik Frèrejean’s picture

Erik Frèrejean
Primary language Dutch
Location 🏴‍☠️
CreditAttribution: Erik Frèrejean at Dutch Open Projects, KennisMeester commented
Status: Needs work » Needs review
Erik Frèrejean’s picture

Erik Frèrejean
Primary language Dutch
Location 🏴‍☠️
CreditAttribution: Erik Frèrejean at Dutch Open Projects, KennisMeester commented
FileSize
2754411-9-11-inderdiff.txt1.15 KB
2754411-11.patch5.92 KB

Noticed a small coding standard violation and the todo from the controller can just as well be removed at this point.

DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna at Mediacurrent commented
FileSize
search_api_page-n2754411-12.interdiff.txt1.93 KB
search_api_page-n2754411-12.patch6.19 KB

A minor improvement - I made the setUpPage() method more reusable.

Status: Needs review » Needs work

The last submitted patch, 12: search_api_page-n2754411-12.patch, failed testing. View results

DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna at Mediacurrent commented
Status: Needs work » Needs review
Related issues: +#3247781: Database update fails, database schema problem on latest release

The automated tests for #12 fail because of #3247781: Database update fails, database schema problem on latest release.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ at Calibrate commented
Status: Needs review » Fixed

Commited and pushed. It improves the situation. Thanks everyone!

Status: Fixed » Closed (fixed)

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