Problem/Motivation

On Uninstall the cookies still stored. We need to delete cookies on uninstall

Steps to reproduce

  1. Install Modal
  2. On Modal Welcome click in "Don't show again"
  3. See the cookies on browser (one cookie is stored)
  4. Uninstall the Modal

Result: The cookie still there
Expected: On uninstall remove all cookies stored by Modal

Proposed resolution

Implemente hook_uninstall and on this hook delete all cookies stores by Modal Page

CommentFileSizeAuthor
#9 interdiff_4-9.txt1.19 KBjnlar
#9 3325910-9.patch971 bytesjnlar
#6 result.png435.81 KBparaderojether
#6 with patch2.png480.63 KBparaderojether
#6 with patch1.png222.44 KBparaderojether
#6 with patch.png626.01 KBparaderojether
#6 without patch1.png346.51 KBparaderojether
#6 without patch.png389.82 KBparaderojether
#4 on-uninstall-the-cookies-still-stored-We-need-to-delete-cookies-on-uninstall-3325910-4.patch939 bytesakhil babu

Issue fork modal_page-3325910

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

RenatoG created an issue. See original summary.

renatog’s picture

renatog commented
Issue summary: View changes
akhil babu’s picture

akhil babu
he/him
commented
Assigned: Unassigned » akhil babu

I will try to Implement this.

akhil babu’s picture

akhil babu
he/him
commented
StatusFileSize
new on-uninstall-the-cookies-still-stored-We-need-to-delete-cookies-on-uninstall-3325910-4.patch939 bytes

I have created a patch. It removes cookies set my modals when we uninstall the module from UI (Extend -> Uninstall). But its not working when module is uninstalled using drush :(

akhil babu’s picture

akhil babu
he/him
commented
Assigned: akhil babu » Unassigned
Status: Active » Needs review

Updating state for reviews and corrections.

paraderojether’s picture

paraderojether commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new without patch.png389.82 KB
new without patch1.png346.51 KB
new with patch.png626.01 KB
new with patch1.png222.44 KB
new with patch2.png480.63 KB
new result.png435.81 KB

Hi RenatoG,

I'm new to this module, I applied path #4 against the Modal module
5.0.x-dev and with core version 9.3 and it is working properly as expected.

Please see the screenshot attached.

Thank You.

renatog’s picture

renatog commented

Thanks a lot for your fix @akhil-babu and for your great review @paraderojether

We'll move it soon

renatog’s picture

renatog commented
Status: Reviewed & tested by the community » Needs work

if (isset($_COOKIE[$cookie_prefix . $modal->id()])) {

I think on Drupal we have a specific function to get cookies instead of using PHP superglobal $_COOKIE right? 🤔

jnlar’s picture

jnlar
Primary language English
Location Sydney, Australia
commented
Status: Needs work » Needs review
StatusFileSize
new 3325910-9.patch971 bytes
new interdiff_4-9.txt1.19 KB

@RenatoG I believe so :^) \Drupal::request()->cookies is one way to interact with session cookies. I've included an updated patch and interdiff from #4.

+function modal_remove_cookie(string $cookie) {
+  $cookies = \Drupal::request()->cookies;
+
+  if ($cookies->has($cookie)) {
+    setcookie($cookie, TRUE, \Drupal::time()->getRequestTime() - 3600, '/');
+    $cookies->remove($cookie);
+  }

Might be overkill, thought it'd remove some repetitiveness.

Should we be creating a test that tests that Modal cookies are removed post installation as well?

renatog’s picture

renatog commented
Status: Needs review » Needs work

Great job, thanks a lot!

I have 2 suggestions:

1) Instead of verify if different than empty like: 

if (!empty($modals)) {
  foreach ($modals as $modal) {
    modal_remove_cookie('please_do_not_show_again_modal_id_' . $modal->id());
    modal_remove_cookie('hide_modal_id_' . $modal->id());
  }
}

I think we can verify if is empty, and if so, skip with early return, you know. E.g.:

if (empty($modals)) {
  return;
}

foreach ($modals as $modal) {
  modal_remove_cookie('please_do_not_show_again_modal_id_' . $modal->id());
  modal_remove_cookie('hide_modal_id_' . $modal->id());
}

With that we can avoid "long conditionals"

2nd suggestion:

I saw that you defined a helper (that is good) but we already have a Helper inside of src folder. So I only suggest move the code of this new helper to there. Makes sense?

sourabhjain’s picture

sourabhjain
Primary language English
commented
Assigned: Unassigned » sourabhjain

Let me start with the suggestion #10.

sourabhjain opened merge request !30

sourabhjain’s picture

sourabhjain
Primary language English
commented
Assigned: sourabhjain » Unassigned

Resolved the suggestion 1 from #10 comment. Created the PR too.
2nd suggestion is still remaining.

jnlar’s picture

jnlar
Primary language English
Location Sydney, Australia
commented
Assigned: Unassigned » jnlar

Thanks all, I will take a look at implementing the 2nd suggestion using the PR branch @sourabhjain created

jnlar’s picture

jnlar
Primary language English
Location Sydney, Australia
commented
Assigned: jnlar » Unassigned
Status: Needs work » Needs review

I've attempted to implement the second suggestion, moving modal_remove_cookie into a method in the ModalPageHelperService service. Cheers for that suggestion @RenatoG, let us know if that looks OK.

sahilgidwani’s picture

sahilgidwani commented
Assigned: Unassigned » sahilgidwani
sahilgidwani’s picture

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

I have checked and reviewed the MR and it is working perfectly for me.

sahilgidwani’s picture

sahilgidwani commented
Assigned: sahilgidwani » Unassigned

renatog’s picture

renatog commented
Status: Reviewed & tested by the community » Fixed
Issue tags: -Novice

It's working fine!

Moved to 5.0.x

Thanks everyone for your contribution effort on this

renatog’s picture

renatog commented
Parent issue: » #3458075: [Roadmap] Plan to Modal 5.0.9

Status: Fixed » Closed (fixed)

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