Follow-up for #2448699-15: Implement caching for the masquerade block and links with a 'masquerade' cache context

Masquerade is using $_SESSION['masquerading'] everywhere, except in the MasqueradeCacheContext::getContext(), where it uses \Symfony\Component\HttpFoundation\Session\Session::has(). The latter is not a wrapper around $_SESSION, and thus always returns FALSE, even when masquerading.

CommentFileSizeAuthor
#24 2688650-24.patch10.79 KBandypost
#24 2688650-interdiff-23.txt1.41 KBandypost
#23 2688650-23.patch11.17 KBandypost
#23 2688650-interdiff-22.txt3.78 KBandypost
#22 2688650-22.patch9.62 KBandypost
#22 2688650-interdiff-20.txt3.71 KBandypost
#20 2688650-20.patch8.54 KBrajeshwari10
#12 2688650-interdiff-6-12.txt3.93 KBandypost
#12 switch_from_using-2688650-11.patch9.29 KBandypost
#9 switch_from_using-2688650-9.patch9.68 KBandypost
#6 switch_from_using-2688650-6.patch5.05 KBandypost
#6 interdiff.txt1.88 KBandypost
#2 switch_from_using-2688650-2.patch3.76 KBmr.baileys
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

andypost created an issue. See original summary.

mr.baileys’s picture

mr.baileys
Primary language Dutch
Location 🇧🇪 (Ghent)
CreditAttribution: mr.baileys at Calibrate commented
Status: Active » Needs review
FileSize
switch_from_using-2688650-2.patch3.76 KB

Patch attached. Session is injected into the Masquerade constructor.

MasqueradeWebTestBase::assertNoSessionByUid() and MasqueradeWebTestBase::assertNoSessionByUid()still rely on the fact that session information is stored in the {sessions} table, and query that table directly. They also use $_SESSION since php's session_decode() is required for unserialization, and it merges into the existing $_SESSION.

If you'd prefer the testing to be session storage-agnostic, we can set up a route in a test module to dump the session info and use that for the assertions?

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented

I think in long run we need to get rid of session storage details in tests
Overall code is great, just test fails again

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented

Tests are broken because of redirect #2448707-11: Fix masquerade tests

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented

If you'd prefer the testing to be session storage-agnostic, we can set up a route in a test module to dump the session info and use that for the assertions?

Looks that only possible way to know session details...

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
FileSize
interdiff.txt1.88 KB
switch_from_using-2688650-6.patch5.05 KB

Re-roll + clean-up

Status: Needs review » Needs work

The last submitted patch, 6: switch_from_using-2688650-6.patch, failed testing.

Ruslan Piskarov’s picture

Ruslan Piskarov
he/him
Primary language Ukrainian
Location Kiev, Ukraine
CreditAttribution: Ruslan Piskarov as a volunteer commented
Status: Needs work » Reviewed & tested by the community

I don't know why the test fails, maybe as @andypost mentioned in#4, however switch_from_using-2688650-6.patch works well for me.

Reviewed and tested manually on my site.
Thank you @andypost.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Status: Reviewed & tested by the community » Needs review
FileSize
switch_from_using-2688650-9.patch9.68 KB

Clean-up patch, added code comments and factored "switchUser" code

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Related issues: +#2881348: SessionCacheContext calls getId() on null

Status: Needs review » Needs work

The last submitted patch, 9: switch_from_using-2688650-9.patch, failed testing. View results

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Status: Needs work » Needs review
FileSize
switch_from_using-2688650-11.patch9.29 KB
2688650-interdiff-6-12.txt3.93 KB

Proper patch

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented

@andypost does the dev branch tests need to be repaired first?

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented

@joelpittet I used to try fix them in #2448707: Fix masquerade tests but no success

then I started to check if getting rid of global can help but ... same

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented

queued to retest https://www.drupal.org/node/49383/qa

Ruslan Piskarov’s picture

Ruslan Piskarov
he/him
Primary language Ukrainian
Location Kiev, Ukraine
CreditAttribution: Ruslan Piskarov as a volunteer commented
Related issues: +#2923960: Convert Simpletest tests to BrowserTestBase

@andypost, in fact, the Unit tests totally were broken and always will fail. Maybe we should review and commit Unit tests before. What do you think about? I added the patch on https://www.drupal.org/project/masquerade/issues/2923960.

Status: Needs review » Needs work

The last submitted patch, 12: switch_from_using-2688650-11.patch, failed testing. View results

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Issue tags: +Needs reroll
rajeshwari10’s picture

rajeshwari10 CreditAttribution: rajeshwari10 as a volunteer and at Valuebound commented
Assigned: Unassigned » rajeshwari10
rajeshwari10’s picture

rajeshwari10 CreditAttribution: rajeshwari10 as a volunteer and at Valuebound commented
Assigned: rajeshwari10 » Unassigned
Status: Needs work » Needs review
FileSize
2688650-20.patch8.54 KB

Hi,

Please Review the patch.

Thanks!!

Status: Needs review » Needs work

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

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
2688650-interdiff-20.txt3.71 KB
2688650-22.patch9.62 KB

Clean-up code comments & fix tests

Symfony stores data in separate bag

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
FileSize
2688650-interdiff-22.txt3.78 KB
2688650-23.patch11.17 KB

A bit more polishing

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Issue tags: +KharkivGSW18
FileSize
2688650-interdiff-23.txt1.41 KB
2688650-24.patch10.79 KB

Remove unused changes

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -KharkivGSW18 +KharkivCS

  • andypost committed 3dfe12a on 8.x-2.x 
    Issue #2688650 by andypost, rajeshwari10, mr.baileys: Switch from using...
andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Status: Reviewed & tested by the community » Fixed

I went ahead and commited current patch, if any bugs will be found report them separate issues

  • andypost committed a3eba01 on 8.x-2.x 
    Issue #2688650 by andypost: Revert context variation by uid

Status: Fixed » Closed (fixed)

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

DanChadwick’s picture

DanChadwick CreditAttribution: DanChadwick commented

Probably warrants a change record and a BREAKING note in the release notes since themes may have been using $_SESSION to influence theming.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented

Thanks, good point, not sure it makes sense now (after 2 years of commit)... but yes, there's interface change as well

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Related issues: +#3042321: Last access by a user should not change after masquerade as that user (D8)

Further changes to session storage