Problem/Motivation

Add SameSite and Secure cookie options to script output

Proposed resolution

Add 2 checkboxes to the general settings to use SameSite=Strict and use_secure_cookies.
Which will add this to the script.

Comment #4 version 1.4.0
Comment #16 version 1.4.1

CommentFileSizeAuthor
#16 3534993-secure-cookies-1.4.1.patch8.8 KBphamnguyen
#13 3534993-secure-cookies.patch8.8 KBphamnguyen
#9 3534993-secure-cookies.patch10.37 KBphamnguyen
#4 3534993-secure-cookies.patch12.72 KBphamnguyen
#2 3534993-secure-cookies.patch9.08 KBphamnguyen

Issue fork piwik_pro-3534993

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

Comments

phamnguyen created an issue. See original summary.

phamnguyen’s picture

phamnguyen commented
StatusFileSize
new 3534993-secure-cookies.patch9.08 KB

Ignore this patch and use the latest in comment #4

phamnguyen opened merge request !24

phamnguyen’s picture

phamnguyen commented
StatusFileSize
new 3534993-secure-cookies.patch12.72 KB
phamnguyen’s picture

phamnguyen commented
Status: Active » Needs review
phamnguyen’s picture

phamnguyen commented
Assigned: phamnguyen » Unassigned
hartsak’s picture

hartsak commented
Version: 1.4.0 » 1.4.x-dev
Status: Needs review » Needs work

Thanks for the effort with this one @phamnguyen and sorry for taking so long with the reply!
I hadn't noticed your work with the issue before.

I added a few comments to the merge request.

Here are a couple of thoughts too:

  • as the 1.4.x branch just changed today, there seems to be some conflicts that must be resolved
  • I appreciate you did the extra work with changing some parts of the code outside the scope of your issue. I think it would still be best to keep the merge request as simple as possible at this point. We can create a new issue where we can clean up the code in general, if it seems necessary. So at this point I suggest those parts to be removed from the merge request which don't seem to be related to the issue in hand.
  • Could you add an update hook for your new boolean settings?
  • Could you add a bit more helpful description texts in the settings form? I mean, maybe mention is it safe to turn those new settings on, or can there be some issues in some cases when the settings are enabled?

Thank you!

phamnguyen’s picture

phamnguyen commented

Thanks for your reply.

  • I will fix the conflicts with the new version.
  • I will undo unnecessary changes which clean up the code.
  • I will add an update hook.
  • I will add extra information to the description of the options.
phamnguyen’s picture

phamnguyen commented
StatusFileSize
new 3534993-secure-cookies.patch10.37 KB

updated patch for version 1.4.1

phamnguyen’s picture

phamnguyen commented

this one is for version 1.4.0

phamnguyen’s picture

phamnguyen commented
Issue summary: View changes
phamnguyen’s picture

phamnguyen commented
Status: Needs work » Needs review
phamnguyen’s picture

phamnguyen commented
StatusFileSize
new 3534993-secure-cookies.patch8.8 KB

wip

phamnguyen’s picture

phamnguyen commented
phamnguyen’s picture

phamnguyen commented
Issue summary: View changes
phamnguyen’s picture

phamnguyen commented
StatusFileSize
new 3534993-secure-cookies-1.4.1.patch8.8 KB

final 1.4.1 version patch

phamnguyen’s picture

phamnguyen commented
Issue summary: View changes
hartsak’s picture

hartsak commented
Status: Needs review » Reviewed & tested by the community

Thanks for all the changes you did to the code @phamnguyen !
I think it looks good now. I tested your changes and everything seems to work.

Even though I previously said we don't want to touch unrelated parts of the module within this issue, I still kind of had to do a small change to the update hooks. They were previously added to the wrong file (.module) and you added the new hook to the correct place (.install file) so I just moved all the old hooks to the .install file, reorganized them a bit and renamed your hook so it should be consistent with the old ones.

I also liked the description texts you had added to the form, great job with those too.

I'll move this to RTBC

  • hartsak committed 89997b1e on 1.4.x authored by phamnguyen 
    [#3534993] feat: Add SameSite and Secure cookie options to script output...
hartsak’s picture

hartsak commented
Status: Reviewed & tested by the community » Fixed

This should be now added to the latest release 1.4.2

Thanks for your contributions @phamnguyen !

Now that this issue is closed, please review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, please credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

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