ISLANDORA-1651: Caching respectful anonymous SESSION handling #101

DiegoPino · 2017-07-31T19:34:37Z

ISLANDORA-1651: https://jira.duraspace.org/browse/ISLANDORA-1651

What does this Pull Request do?

Islandora bookmark allows anonymous users to manage Bookmarks by displaying a Block to add PIDS and keeping Bookmark info available using the global $_SESSION PHP variable. When that happens, Drupal initialises a full blown SESSION using SESSxxxx cookie,
(see https://api.drupal.org/api/drupal/includes!bootstrap.inc/function/drupal_settings_initialize/7)
same used by Drupal to track current logged in users.

This prevents caching systems like Varnish or Memcached to detect correctly if a page is being viewed by an anonymous user (and candidate for caching) or by a logged User (different caching strategy, if any).

A Drupal session in place for anonymous users adds performance penalties. Read
http://facingworlds.com/drupal-7-bootstrap-explained-drupalbootstrapsession-and-drupals-session-handling.

This PULL request avoids initializing the SESSION in question at all when not needed and only makes use of it when an Anonymous user actually saves something to a Bookmark session, instead of when just exposed to the functionality as before. This fix has been in production at DCMNY.org for over a year already.

Relevant to cookie creation:
https://github.com/Islandora/islandora_bookmark/pull/101/files#diff-3196d324ec7e08230f97233bfabca6ffR95

What's new?

Islandora Bookmark Drupal Block for Anonymous users can be used without caching penalties.

How should this be tested?

If running under Varnish/Memcached and without this fix, verify Session cookies when the Bookmark Block provided by this module is enabled for anonymous users. You should see SESSxxxx even if not using bookmarks at all, just by accessing an object's page that displays the block. Look at your caching headers. You will see that Varnish simply skips caching once the Block is displayed (even once) for all the lifespan of that cookie, on any page of an islandora Site.

Enable this module and reset your browser ( the previous cookie will survive even an Atomic bomb). make sure the cookie is gone. All existing functionality should still work, but if anonymous does not add an Object to PID then SESSxxxx should not be around.

If you want to be even finer, you can test page loading times with and without patch when displaying the Bookmark Block. With the Fix it should be faster, the amount will vary of course.

Additional Notes:

This Pull request includes a lot of Drupal Coding Standards fixes because:

drupal/coder 8.2.12
squizlabs/php_codesniffer 2.9.1

Does this change require documentation to be updated?
No
Does this change add any new dependencies?
No
Does this change require any other modifications to be made to the repository (ie. Regeneration activity, etc.)?
No
Could this change impact execution of existing code?
No

Interested parties

@Islandora/7-x-1-x-committers @jonathangreen @adam-vessey @jordandukart

BookmarkDefaultSessionList::createNewList is being called with completely wrong arguments, passing the user as description (and storing it into the session)

This change pushes the $_SESSION creation logic deeper into the logic of the module, allowing anonymous users not making actual use of bookmarks to keep making use of existing cache. Also fixes some bugs.

Clean Up 1/X

2/X

3/X

4/x

5/x and some t() issues. See https://www.drupal.org/docs/7/api/localization-api/dynamic-or-static-lin ks-and-html-in-translatable-strings

jonathangreen

Haven't gone through this in detail yet, just noticed this quickly.

jonathangreen · 2017-07-31T19:50:45Z

islandora_bookmark.module

    $form['islandora_bookmark']['lists'] = array(
      '#type' => 'item',
-      '#prefix' => '<h3>' . t('Bookmarked in') . ':</h3>',
-      '#markup' => theme('item_list', array('items' => $links)),
+      '#prefix' => t('<h3>Bookmarked in:</h3'),


Missing closing tag here.

Wouldn't this change existing translations, since its changing whats in the T?

@jonathangreen it is giving me
"Do not concatenate strings to translatable strings,
they should be part of the t() argument and you should use placeholders
". wonder if i should use place holders.. maybe? See https://www.drupal.org/docs/7/api/localization-api/dynamic-or-static-links-and-html-in-translatable-string?

I got away using #prefix and #sufix on the other few cases, but not possible here really.

@jonathangreen 7a598e4 should fix the issue so translation stays stable. Only way to have coder pass.

This will avoid having to -retranslate existing bits for “Bookmarked in”, note that : is kept out as before.

jonathangreen

These changes are good. They solve the issue where if an anonymous user can use bookmarks, then they will never be able to use cached pages, even if they never use bookmark. That functionality works great.

I see a few issues, all somewhat related.

When toggling variable_get('islandora_bookmark_create_user_default_lists', FALSE) the default list is always displayed
anonymous users can no longer create new lists
the default list disappears when a new list is created

jonathangreen · 2017-08-01T18:07:15Z

includes/api.inc

@@ -38,14 +38,20 @@ function islandora_bookmark_get_user_owned_bookmarks($type = NULL) {

  // User is anon, use session as well.
  if ($user->uid == 0) {
-    if (!isset($_SESSION) || empty($_SESSION['islandora_bookmark']) && variable_get('islandora_bookmark_create_user_default_lists', FALSE)) {
-      BookmarkDefaultSessionList::createNewList(t('My Default List'), 'bookmark_default', $user);
+    if (!isset($_SESSION) || !isset($_SESSION['islandora_bookmark']) || empty($_SESSION['islandora_bookmark']) && variable_get('islandora_bookmark_create_user_default_lists', FALSE)) {


This will always show the default list even if the variable islandora_bookmark_create_user_default_lists is set to false. This is a regression from the old module.

This section also causes a problem when an anonymous user creates a list. If there is nothing stored in the default list it just disappears at that point, when it should stay in the list if variable_get('islandora_bookmark_create_user_default_lists', FALSE) is TRUE.

@jonathangreen true.This pull was made probably before. Can´t remember. But can fix that.

@jonathangreen i'm looking at the logic right now.
According to myself:

if ( ( // Open "(" !isset($_SESSION) || // No SESSION !isset($_SESSION['islandora_bookmark']) || // OR at least not a SESSION with an islandora_bookmark entry empty($_SESSION['islandora_bookmark']) // OR it is there but it is empty )//close ")" && variable_get('islandora_bookmark_create_user_default_lists', FALSE)) // and create default list is TRUE

So adding extra () around the session conditionals should be ok?. Which would mean in human language:
If all the session stuff is empty AND i´m asked to create a default list (evaluated to TRUE), then do it.

I wonder if you would be ok if I move all the anonymous user functionality directly into cookies? The original patch was to avoid the issue in a less disruptive way (seems like I had no luck in that department! but I read that there are a lot of expectations for anonymous user bookmark lists management. Like renaming and adding new ones. It's a bit novel for me since that type of behavior is normally reserved to logged in users in other systems, really can't find right now any other system hat allows so much freedom.

jonathangreen · 2017-08-01T18:23:05Z

includes/api.inc

    }

    if (!empty($_SESSION['islandora_bookmark'])) {
      foreach (array_keys($_SESSION['islandora_bookmark']) as $id) {
        $bookmark_list[] = BookmarkSession::getList($id);
      }
+      if (count($_SESSION['islandora_bookmark']) == 1) {


This section poses problems if you try to create a list as an anonymous user. Since when you create a list it is always empty, this immediately deletes the list that was just created.

I need to look at that because I can´t remember really the exact "why" of this. I slightly remember thinking anonymous users, which are a "bookmarking" burden, should not trust or rely on having multiple lists around and having them to be kept safely around because it depends on cookies) and we don´t even notify them that this storage is just session based and volatile. But again, need to dig deeper to know the why? Will have a solution/answer tomorrow. Thanks!

Original implementation details was dealing with an end user who may have forgotten to log in before doing actions. Granted I 100% agree that session based management is not the correct route but turfing functionality is a tough sell imo.

Will work on keeping existing use case alive. Just avoiding creation a session just because the form is presented. Thanks!

@jonathangreen ok, true. Legacy code to remove stuck sessions and i don´t find any self notes that make that piece relevant anymore. Will fix somehow, i do need a functionality to clear $_SESSIONS if created and not in use to avoid stuff being carried over and over, but this is not the way. Also, if i move all to user cookies...

DiegoPino · 2017-08-10T18:55:27Z

@jonathangreen still on this, have not forgotten it, just slow, very slow here.

jordandukart · 2017-08-22T17:56:04Z

islandora_bookmark.module

@@ -662,7 +668,7 @@ function islandora_bookmark_detailed_form_manage(array $form, array &$form_state
  $bookmark = Bookmark::getList($list_id);

  // Manage.
-  if ($bookmark->bookmarkOwner === $user->uid) {
+  if ($bookmark->bookmarkOwner === $user->uid && $bookmark->getIsDeletable()) {


Does this not limit some actions below from being changed such as the name and descriptions, seeing as there is in access check in https://github.com/Islandora/islandora_bookmark/pull/101/files#diff-119271b109bd4f411bd92328c14d451eR697

@jordandukart will look at it tomorrow. I have some new code to pull in here to address @jonathangreen concerns. Also, some of this is a bit old and needs to brought up to date.

DiegoPino · 2017-08-24T17:45:18Z

@jonathangreen. Side note, to debug islandora_usage_stats (vangrant) needs to be disabled, SESSION is used there too! https://github.com/ryersonlibrary/islandora_usage_stats/blob/d1bf50a62f2cc92a58fa882268bd48bc125d750c/includes/utilities.inc#L119-L124

Anyway. I will move 100% to cookies for anonymous users. This swapping between SESSIONS and cookies makes everything even more complicated. I noticed a security concern also with the current (not this pull) implementation. We don´t have a limit at all for how many anonymous lists can be created. Well happens that I could basically mimic n+1 submissions and fill a SESSION database with data that will persist for a long time (happens that there is no session invalidation per default since there is no log out for anonymous). Have this stuff on my cutting bench now, so please be patient until I got it solved, thanks

jordandukart · 2018-03-05T15:20:12Z

Hey @DiegoPino any movement on this?

DiegoPino · 2018-03-05T15:30:33Z

@jordandukart will be done before code freeze. Got derailed but I have a working solution that addresses @jonathangreen concerns, but now I need to clean it up. If you have any ongoing/needed pulls you want to push now, go on, I can always rebase. Thanks

DiegoPino · 2018-03-05T15:32:57Z

@jordandukart also, sorry, time flies and I'm over committed with other stuff. I was not even aware this is from August. Will deal with this before our next committers call.

jordandukart · 2018-03-05T15:36:09Z

No worries @DiegoPino, nothing pressing on our end!

bondjimbond · 2018-10-19T19:43:55Z

@DiegoPino Is this PR still relevant?

DiegoPino · 2018-10-19T20:21:48Z

Yes, it is. Not a simple issue to fix, but it is.

DiegoPino · 2018-11-20T18:28:48Z

Re assigning to me.... i will fix it this week (year 2018)

DiegoPino and others added 7 commits May 30, 2016 21:40

Fix invalid signature call, major bug

47b99c0

BookmarkDefaultSessionList::createNewList is being called with completely wrong arguments, passing the user as description (and storing it into the session)

Move $_SESSION creation to where it is really needed

5a40d53

This change pushes the $_SESSION creation logic deeper into the logic of the module, allowing anonymous users not making actual use of bookmarks to keep making use of existing cache. Also fixes some bugs.

Legacy Coding Standards

14a162b

Clean Up 1/X

Legacy Coding Standards

9dc6e4a

2/X

Legacy Coding Standards

9c2721d

3/X

Legacy Coding Standards

cec85b1

4/x

Legacy Coding Standards

927eded

5/x and some t() issues. See https://www.drupal.org/docs/7/api/localization-api/dynamic-or-static-lin ks-and-html-in-translatable-strings

DiegoPino changed the title ~~Anonymous~~ ISLANDORA-1651: Caching respectful anonymous SESSION handling Jul 31, 2017

jonathangreen self-assigned this Jul 31, 2017

jonathangreen self-requested a review July 31, 2017 19:46

jonathangreen removed their assignment Jul 31, 2017

jonathangreen suggested changes Jul 31, 2017

View reviewed changes

DiegoPino added 2 commits July 31, 2017 16:03

code-blindness is a desease

e01fbf0

Better translate-able piece

7a598e4

This will avoid having to -retranslate existing bits for “Bookmarked in”, note that : is kept out as before.

jonathangreen suggested changes Aug 1, 2017

View reviewed changes

jordandukart reviewed Aug 22, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ISLANDORA-1651: Caching respectful anonymous SESSION handling #101

ISLANDORA-1651: Caching respectful anonymous SESSION handling #101

DiegoPino commented Jul 31, 2017 •

edited

Loading

jonathangreen left a comment

jonathangreen Jul 31, 2017

jonathangreen Jul 31, 2017

DiegoPino Jul 31, 2017

DiegoPino Jul 31, 2017

DiegoPino Aug 1, 2017

jonathangreen left a comment

jonathangreen Aug 1, 2017

jonathangreen Aug 1, 2017

DiegoPino Aug 1, 2017

DiegoPino Aug 24, 2017

jonathangreen Aug 1, 2017

DiegoPino Aug 1, 2017 •

edited

Loading

jordandukart Aug 22, 2017

DiegoPino Aug 22, 2017

DiegoPino Aug 24, 2017

DiegoPino commented Aug 10, 2017

jordandukart Aug 22, 2017

DiegoPino Aug 22, 2017

DiegoPino commented Aug 24, 2017

jordandukart commented Mar 5, 2018

DiegoPino commented Mar 5, 2018

DiegoPino commented Mar 5, 2018

jordandukart commented Mar 5, 2018

bondjimbond commented Oct 19, 2018

DiegoPino commented Oct 19, 2018

DiegoPino commented Nov 20, 2018

ISLANDORA-1651: Caching respectful anonymous SESSION handling #101

Are you sure you want to change the base?

ISLANDORA-1651: Caching respectful anonymous SESSION handling #101

Conversation

DiegoPino commented Jul 31, 2017 • edited Loading

What does this Pull Request do?

What's new?

How should this be tested?

Additional Notes:

Interested parties

jonathangreen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonathangreen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DiegoPino Aug 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DiegoPino commented Aug 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DiegoPino commented Aug 24, 2017

jordandukart commented Mar 5, 2018

DiegoPino commented Mar 5, 2018

DiegoPino commented Mar 5, 2018

jordandukart commented Mar 5, 2018

bondjimbond commented Oct 19, 2018

DiegoPino commented Oct 19, 2018

DiegoPino commented Nov 20, 2018

DiegoPino commented Jul 31, 2017 •

edited

Loading

DiegoPino Aug 1, 2017 •

edited

Loading