Cache limiter support attempt #12

pine3ree · 2018-04-26T02:37:58Z

Inject cache headers based on cache_limiter value, while removing the automatic job from the php engine.

The php engine automatically sends cache-http-headers based on the values of session.cache_limiter, session.cache_expire ini/session_start settings and the script last mtime.

The idea is to steal the value of session.cache_limiter inside the constructor and set an empty cache_limiter when starting the session, thus disabling the automatic headers.

Then we recreate the logic of the zend-engine to build the headers ourselves and inject them into the response.

TODO/TOCHECK

find a better/proper way to check the running script mtime
decide if/when to inject the cache header: right now if a cache header is found we assume that it was added programmatically in an internal middleware layer, so we skip injecting any of them. What should we do: override-headers? add-(multi-value)-headers?

…ame param

pine3ree · 2018-04-26T04:00:44Z

need to fix lowered coverage...

…ecked before call)

pine3ree · 2018-04-26T05:22:25Z

Fixes #10

weierophinney · 2018-04-26T13:27:00Z

src/PhpSessionPersistence.php

+use function sprintf;
+use function gmdate;
+use function time;
+use function filemtime;


Import statements should be alphabetized (simplifies finding declarations, and reduces chances of duplication).

sorry, that's ignorance on my part...I always used to set the order based on which func was called first

No worries - just providing some guidance. :)

weierophinney · 2018-04-26T13:27:33Z

src/PhpSessionPersistence.php

+        'private_no_expire' => true,
+    ];
+
+    public const CACHE_PAST_DATE  = 'Thu, 19 Nov 1981 08:52:00 GMT';


Why this date in particular, and not the unix epoch?

I was curious about that value as well 😄
https://github.com/php/php-src/blob/master/ext/session/session.c#L1194

Wow, that's super bizarre! I guess we should use the value for consistency with the engine... maybe throw a comment in indicating where we get the value from, though. :)

Yeah, I used it for consistency....but I'm still wondering where (or when) it comes from....

Probably some PHP contributor's birthday. 😀

https://stackoverflow.com/questions/8194481/why-is-expires-1981 :)

weierophinney · 2018-04-26T13:31:22Z

src/PhpSessionPersistence.php

+                }
+            }
+
+            return $response;


Invert the conditional so you can return early, combine conditions, and promote the body of the conditional:

if (! $this->cacheLimiter || $this->responseAlreadyHasCacheHeaders($response)) { return $response; } $cacheHeaders = $this->generateCacheHeaders($this->cacheLimiter, $this->cacheExpire); // ... return $response;

weierophinney · 2018-04-26T13:32:01Z

src/PhpSessionPersistence.php

+    /**
+     * Generate cache headers for a given session cache_limiter value.
+     * @param string $cacheLimiter
+     * @param int $cacheExpire


Parameter annotations are discouraged unless they add descriptions; these can be removed.

do I keep the method's purpose explaination?

weierophinney · 2018-04-26T13:33:23Z

src/PhpSessionPersistence.php

+        }
+
+        $maxAge       = 60 * $cacheExpire;
+        $lastModified = $this->getLastModified($_SERVER['SCRIPT_FILENAME'] ?? '');


We typically try not to access superglobals directly. Please pass this value to the method by using the following from within initializeSessionFromRequest():

$request->getServerParams()['SCRIPT_FILENAME']

Considering that this may not be set, you'll need to come up with a default value to use; in that case, I'd likely use __FILE__:

$request->getServerParams()['SCRIPT_FILENAME'] ?? __FILE__

Yeah, I thought about that too...I was actually waiting for your review. I actually used __FILE__ in the test case

I'd have a few questions before applying the changes:

generateCacheHeaders(...) is privately used only with internal properties, should I remove the arguments and use them directly?

same question getLastModified, since the scriptFile will be saved into an internal property

Should I remove thi PR and re-add a 1-commit PR? (I started uploading to upstream while forgetting i had already opened the PR, I apologize for the number of commits)

Sure, that should work fine.

Same.

No, I'm fine with multiple commits. If YOU are not, then run git rebase -i and squash some commits and force push back to your branch.

weierophinney · 2018-04-26T13:38:04Z

src/PhpSessionPersistence.php

+    /**
+     * Check if the response already carries cache headers
+     * @param ResponseInterface $response
+     * @return bool


Both of these annotations may be removed, as they duplicate the typehints.

weierophinney · 2018-04-26T13:38:30Z

src/PhpSessionPersistence.php

+    private function responseAlreadyHasCacheHeaders(ResponseInterface $response) : bool
+    {
+        return (
+               $response->hasHeader('Expires')


Reduce the indentation of this line; || operators should align with this line.

weierophinney · 2018-04-26T13:38:49Z

test/PhpSessionPersistenceTest.php

@@ -23,6 +23,8 @@
 use function session_name;
 use function session_start;
 use function session_status;
+use function time;
+use function gmdate;


Alphabetize imports, please.

weierophinney · 2018-04-26T13:44:20Z

test/PhpSessionPersistenceTest.php

+        $expires = strtotime($expires);
+
+        $this->assertTrue($expires >= $expiresMin);
+        $this->assertTrue($expires <= $expiresMax);


Please use assertGreaterThanOrEqual() and/or assertLessThanOrEqual().

weierophinney · 2018-04-26T13:46:29Z

test/PhpSessionPersistenceTest.php

+
+        // temporarily set SCRIPT_FILENAME to current file for testing
+        $scriptFilename = $_SERVER['SCRIPT_FILENAME'] ?? null;
+        $_SERVER['SCRIPT_FILENAME'] = __FILE__;


With the changes I suggested earlier around usage of superglobals, this test will need to change.

What I didn't realize is that you're accessing it during persistSession() as well. I'm wondering if the script filename should be memoized in the Session instance, and removed during persistence?

yes, see my previous reply...I don't think we need to remove it though whit it being refreshed at every request...and now that I think about it...should we just memoize it once....the running script is not going to change even in asynchronous contexts?

....mhmmm....but mocked requests could contain a different SCRIPT_FILENAME....

pine3ree · 2018-04-26T14:07:39Z

..i forgot another question:
I used public constants for past date and http date format and reused them in the tests, but, aside from the tests, I don't they are useful outside the class

weierophinney · 2018-04-26T14:10:25Z

I used public constants for past date and http date format and reused them in the tests, but, aside form the tests, I don't they are useful outside the class

Using them in tests is public behavior, so you can mark them public if you want. Otherwise you'll need to use reflection in your tests to get at the values, which is generally not fun. :)

pine3ree · 2018-04-26T16:26:44Z

mhmm...I'm not sure why test are failing on travis, everything works locally...I'll check later....

pine3ree · 2018-04-26T17:25:32Z

Still no clue, locally all test pass and in a test app the appropriate headers are correctly sent.
Could it be a travic issue?
What I find weird is that even a simple unchanged test case like the cache_limiter === nocache is failing, even if the session config is memoized as before and the code generating that constant headers array is basically unchanged (just remove the arguments and use the internal properties directly)

pine3ree · 2018-04-27T01:15:02Z

@weierophinney

Travis build continue to fail, I believe this has smt to do with the merging of my previous (unrelated) merged PR, even if there is no report of conflicts. My same fork and another test branch forked from it are still passing tests successfully. I also see a strange decrease in coverage.

https://travis-ci.org/pine3ree/zend-expressive-session-ext/builds/371829367

any idea?
kind regards

PS
there is another small change that I believe could make sense.

https://github.com/pine3ree/zend-expressive-session-ext/blob/00e0f7c9a3af7c0cc8ae4a456e83438946c1388f/src/PhpSessionPersistence.php#L222

In the getLastModified() method we may have a memoized scriptFile either from a request server param or from the current __FILE__. In cases when the request scritpFile is an empty string or representes a non existent file, we still end up with a false value. I believe we should set it to __FILE__ for those cases as well inside the function.

In the end we should always get a value, but since gmdate could also return false, we should assume that value can be returned as well and leave the fals check when injecting the headers

    //....
    public function initializeSessionFromRequest(ServerRequestInterface $request) : SessionInterface
    {
         // comment note: we set it to __FILE__ only once inside getLastModified if necessary
        $this->scriptFile = $request->getServerParams()['SCRIPT_FILENAME'] ?? null;
        //....
    }
    //....
    private function getLastModified()
    {
        if (! $this->scriptFile || ! is_file($this->scriptFile)) {
            $this->scriptFile = __FILE__;
        }

        return gmdate(self::HTTP_DATE_FORMAT, filemtime($this->scriptFile));
    }
    //----

(edited)
The test cases were not compatible with the last merge, I fixed it. regards.

pine3ree · 2018-05-02T01:56:58Z

Hello @weierophinney,
I rebased to the latest merge and squashed into fewer commits for a cleaner history into a new branch:

https://github.com/pine3ree/zend-expressive-session-ext/tree/cache-limiter
(replacing current PR cache-limiter-support)
https://github.com/pine3ree/zend-expressive-session-ext/branches
(patch-1 and patch-2 are unrelated to this matter, already opened different PRs for them)
travis tests
compare + cleaner-history

When you review last commits/comments pls let me also know if you prefer a PR from the new branch (closing this one)

kind regards

weierophinney

Final changes look perfect, @pine3ree !

Cache limiter support attempt

Close #12

weierophinney · 2018-05-10T17:25:33Z

Thanks, @pine3ree! Merged to develop for release with 1.1.0.

pine3ree added 8 commits April 26, 2018 04:20

Inject cache headers based on cache_limiter value

789082c

pushing tests to upstream

4d4ff38

remove redundant assignments

1925420

add space after not operator

1335f8d

change generateCacheHeaders signature

c62d364

add return types + change getLastModified() signature to accept filen…

7945a4e

…ame param

added test for empty cache_limiter

38c42b8

added last-modified test

b75f95b

pine3ree mentioned this pull request Apr 26, 2018

Question about session cache response header #10

Closed

pine3ree added 4 commits April 26, 2018 06:17

added last-modified failure test

2649c26

added unsupported cache_limiter value test

bb74817

update getLastModified method: if is_file can do filemtime

726f701

fix last coverage failure: condition is never met (cacheLimiter is ch…

7506e66

…ecked before call)

pine3ree added 2 commits April 26, 2018 13:26

add supported limiters property and de-uglify code

e08ebe0

fix cs space after not

90114ef

weierophinney suggested changes Apr 26, 2018

View reviewed changes

Implement changes requested by the reviewer (good man)

c936c6d

remove unused argument: changed signature

08e26a4

pine3ree added 3 commits April 26, 2018 20:43

re-add return type anotation and re-trigger travis tests

fd89643

trigger travis tests

27423f4

remove redundant retun and trigger new travis test

5ab6ed7

pine3ree added 2 commits April 27, 2018 22:12

fix test cases for compatibility with recently merged fix

2fec15d

add and use helper methods to improve test cases readability

775598a

weierophinney approved these changes May 10, 2018

View reviewed changes

weierophinney added a commit that referenced this pull request May 10, 2018

Merge pull request #12 from pine3ree/cache-limiter-support

2f1b3ae

Cache limiter support attempt

weierophinney added a commit that referenced this pull request May 10, 2018

Adds CHANGELOG entry for #12

aea9627

weierophinney added a commit that referenced this pull request May 10, 2018

Merge branch 'feature/12' into develop

c8223cc

Close #12

weierophinney closed this May 10, 2018

pine3ree deleted the cache-limiter-support branch May 11, 2018 02:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cache limiter support attempt #12

Cache limiter support attempt #12

pine3ree commented Apr 26, 2018

pine3ree commented Apr 26, 2018

pine3ree commented Apr 26, 2018 •

edited by weierophinney

Loading

weierophinney Apr 26, 2018

pine3ree Apr 26, 2018

weierophinney Apr 26, 2018

weierophinney Apr 26, 2018

pine3ree Apr 26, 2018

weierophinney Apr 26, 2018

pine3ree Apr 26, 2018

weierophinney Apr 26, 2018

pine3ree Apr 26, 2018

weierophinney Apr 26, 2018

weierophinney Apr 26, 2018

pine3ree Apr 26, 2018

weierophinney Apr 26, 2018

weierophinney Apr 26, 2018

pine3ree Apr 26, 2018 •

edited

Loading

weierophinney Apr 26, 2018

weierophinney Apr 26, 2018

weierophinney Apr 26, 2018

weierophinney Apr 26, 2018

weierophinney Apr 26, 2018

weierophinney Apr 26, 2018

pine3ree Apr 26, 2018

weierophinney Apr 26, 2018

pine3ree Apr 26, 2018

pine3ree commented Apr 26, 2018 •

edited

Loading

weierophinney commented Apr 26, 2018

pine3ree commented Apr 26, 2018

pine3ree commented Apr 26, 2018

pine3ree commented Apr 27, 2018 •

edited

Loading

pine3ree commented May 2, 2018 •

edited

Loading

weierophinney left a comment

weierophinney commented May 10, 2018

Cache limiter support attempt #12

Cache limiter support attempt #12

Conversation

pine3ree commented Apr 26, 2018

pine3ree commented Apr 26, 2018

pine3ree commented Apr 26, 2018 • edited by weierophinney 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

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

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

pine3ree Apr 26, 2018 • 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

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

pine3ree commented Apr 26, 2018 • edited Loading

weierophinney commented Apr 26, 2018

pine3ree commented Apr 26, 2018

pine3ree commented Apr 26, 2018

pine3ree commented Apr 27, 2018 • edited Loading

pine3ree commented May 2, 2018 • edited Loading

weierophinney left a comment

Choose a reason for hiding this comment

weierophinney commented May 10, 2018

pine3ree commented Apr 26, 2018 •

edited by weierophinney

Loading

pine3ree Apr 26, 2018 •

edited

Loading

pine3ree commented Apr 26, 2018 •

edited

Loading

pine3ree commented Apr 27, 2018 •

edited

Loading

pine3ree commented May 2, 2018 •

edited

Loading