Allow the session name suffix to be configurable

The Needs Review Queue Bot → tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Re-roll Patch #56 against Drupal 10.1.x

Fixed failed commands on #61
Attached patch against Drupal 10.1.x
Attached interdiff

Unfortunately, I have patched the core files (#56 for 9.5) but the reset password process lead to an access denied page. Removing the D7 cookies solve this issue.

@DuneBL Did you configure a session suffix after installing the patch?

No, I didn't... and I have no clue on how to do it... many thanks for pointing me to this... If you can provide me some explanation it would be great

Copy default.services.yml to services.yml, if you don't have one. Edit, or add a session_suffix key after the sid_bits_per_character key. Put a short string of characters in it. That's how this patch works, but it lacks documentation at this time.

In my opinion, for clarity the configuration key and its reference in code should be session_name_suffix rather than session_suffix.

In fact name_suffix would be short and accurate, because the context is clearly about sessions.

@cilefen many thanks it is working. I confirm patch #56 is working on 9.5

Corrected re-roll errors in #61 that fed into #62.

(patch written for 10.1.x, but also applies to 11.x)

Would be good to see the issue summary updated to the standard template,

Previously tagged for a change record.

Recommend using MRs

Re-roll for D10.2

I prefer the name I suggested in comment #69.

Thanks all for working on this! I have converted the latest patch to the MR. Also updated the name as per #69.

Draft CR is here: https://www.drupal.org/node/3446100 →

I think this issue is pretty important to fix, as with the D7 EOL approaching, this has potential to cause a lot of troubles to sites still going to migrate. We were also caught by this on a few sites and no user with a D7 cookie was able to login until the cookie was deleted.

Pipeline seems to be green, moving to Needs review.

Slight title update as assuming this is an issue no matter the landing version.

Reviewing the issue summary solution and seems straight forward about adding the suffix, also reviewed the CR and reads fine to me.

Reviewing the code changes and appears to be doing as advertised, test coverage update seems to be there too.

Believe this is probably good.

I reviewed the comments and I think they need improvement. Also, the title is the statement of the problem not what is being changed. The change record title is more what I am thinking. Although the change record does not need Drupal in the title because it is a change record for Drupal :-).

The change record title could be 'The session_name suffix can be configured.' and the issue title "Allow the session_name suffix to be configurable'

Finally, should it be 'session name' or 'session_name' in the documentation. I'd also like to know why if it a session name suffix why not name the new property 'session_name_suffix'?

It should be name_suffix. The entire context is the session.

The full name is session.storage.options.name_suffix, which to me makes more sense than session.storage.options.session_name_suffix.

I have updated the comments in the MR (still open to further suggestions), issue title and the CR.

I have not used session_name (with underscore) in those sentences, but instead used session name, because from what I have seen, we use this phrase without underscore on different places in code already (phrase session_name is only used as a $session_name variable). So I think it would be better without the underscore.

Also agree with @cilefen, that the name_suffix is probably sufficient, as it is under the session.storage.options.

Thanks!

Actually after the suggested title change, it may look like this is no longer a "Bug report", but I suggest this is still treated as a bug, because it is still a broken functionality related to D7 -> D10 upgrade (and pretty major).

There is an unrelated MR pipeline failure. It seem to be related to this: 🐛 Composer tests fail because of invalid Drupal version Fixed

Therefore keeping this as Needs Review.

From what I can tell the feedback has been addressed. The added comment definitely clearly states what this is for and does.

There are failed tests - can the MR be updated for the latest changes and have a green test run.

Rebased and the tests are now green, so setting the status to RTBC.

This looks good to me now. Committed/pushed to 11.x and cherry-picked to 11.0.x, 10.4.x, 10.3.x, thanks!

catch → closed merge request !7971

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