The fix for issue 56357 was meant to ensure that each Drupal site in a domain has a unique session name, but it causes different session names depending on whether HTTP or HTTPS is being used.

This patch was moved from issue #159854. Thanks to Darren Oh for reporting the bug and to moshe weitzman for marking it RTBC.

CommentFileSizeAuthor
#11 protocol-session_0.patch856 bytesjohnalbin
#9 protocol-session.patch845 bytesjohnalbin
#7 schema-session_1.patch812 bytesjohnalbin
#1 schema-session_0.patch701 bytesjohnalbin
base-url_1.patch3.47 KBjohnalbin

Comments

johnalbin’s picture

johnalbin
they/他
Primary language English
Location Taipei, Taiwan
commented
StatusFileSize
new schema-session_0.patch701 bytes

I attached the wrong patch. Here is the correct one.

darren oh’s picture

darren oh
Primary language English
Location Lakeland, Florida
commented

I included this in the final version of the patch for issue 159854.

johnalbin’s picture

johnalbin
they/他
Primary language English
Location Taipei, Taiwan
commented

Darren, I’d like to get this back-ported to 5.x quickly. And since it’s RTBC, there’s no point in waiting on 159854.

darren oh’s picture

darren oh
Primary language English
Location Lakeland, Florida
commented

I'd prefer a patch that fixes everything that the patch for issue 56357 broke. Based on your input, I have updated the patch in issue 159854 so that it does so. If it's ready to be committed, this issue should be marked as a duplicate.

johnalbin’s picture

johnalbin
they/他
Primary language English
Location Taipei, Taiwan
commented

It‘s quite simple. There are 2 separate issues with the fix in 56357. This issue is RTBC and 159854 needs work/review. Having a separate patch for each issue is a best practice in software development.

drumm’s picture

drumm
he/him
Location NY, US
commented

This looks okay, but needs a code comment explaining what is happening.

johnalbin’s picture

johnalbin
they/他
Primary language English
Location Taipei, Taiwan
commented
StatusFileSize
new schema-session_1.patch812 bytes

Added a comment. Otherwise, the patch is identical.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Reviewed & tested by the community » Needs work

I'd do the following, which is easier to read and understand IMHO (both in the comment and in the code):

// Otherwise use $base_url as session name, without the protocol
// to use the same session identifiers across http and https.
list($session_name) = explode('://', $base_url, 2);

We could also use substr(strpos()...), but I fear "coloring the bikeshed" with nitpicking...

johnalbin’s picture

johnalbin
they/他
Primary language English
Location Taipei, Taiwan
commented
Status: Needs work » Needs review
StatusFileSize
new protocol-session.patch845 bytes

Gabor, I assume you meant list( , $session_name) = ….

Here’s the new blue-colored bike shed patch. I‘ve tested this on my system to make sure it works like the previous patch.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Version: 6.x-dev » 5.x-dev
Status: Needs review » Patch (to be ported)

OK, this bike shed looks much better :) Committed to Drupal 6, as Niel indicated, it should go into Drupal 5 as well.

johnalbin’s picture

johnalbin
they/他
Primary language English
Location Taipei, Taiwan
commented
Status: Patch (to be ported) » Reviewed & tested by the community
StatusFileSize
new protocol-session_0.patch856 bytes

6.x patch applies with “offset -24 lines,” but here’s a re-rolled patch that applies cleanly.

johnalbin’s picture

johnalbin
they/他
Primary language English
Location Taipei, Taiwan
commented

I also ran my Zend PHP debugger and verified that $session_name gets set to $base_url minus the [protocol]://

drumm’s picture

drumm
he/him
Location NY, US
commented
Status: Reviewed & tested by the community » Fixed

Committed to 5.x.

Anonymous’s picture

(not verified) commented
Status: Fixed » Closed (fixed)