New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor session scope usage into a series of overridable methods #467

Merged
merged 2 commits into from Feb 22, 2017

Conversation

Projects
None yet
2 participants
@atuttle
Contributor

atuttle commented Feb 21, 2017

Per discussions in slack, this seemed like a welcome contribution.

This is my first attempt at it, but I'm quite open to feedback and suggestions for improvement. In my case we'll be overriding these methods to work with session data going into Redis instead of ACF's session scope.

So far I have made the changes you see here and tested that everything still seems to be in working order in my (somewhat large) app. Once I was happy with these changes I overrode these new extension points and swapped in Redis functionality. I'm using sidecar to manage them, but my examples below should be reasonably readable pseudocode even not knowing how I'm hooking everything up to Redis.

Here's the code I've added to my Application.cfc to override the new extension points:

	public void function sessionDefault( string keyname, string defaultValue ) {
		var tmp = application.sessionAdapter.get( arguments.keyName, arguments.defaultValue );
		application.sessionAdapter.set( arguments.keyName, tmp );
	}

	public void function sessionDelete( string keyname ) {
		application.sessionAdapter.clear( arguments.keyname );
	}

	public boolean function sessionHas( string keyname ) {
		return application.sessionAdapter.has( arguments.keyname );
	}

	public void function sessionLock( required function callback ) {
		lock name="sessionLockAppCFC" type="exclusive" timeout="30" {
			callback();
		}
	}

	public any function sessionRead( string keyname ) {
		return application.sessionAdapter.get( arguments.keyname );
	}

	public void function sessionWrite( string keyname, any keyvalue ) {
		application.sessionAdapter.set( arguments.keyname, arguments.keyvalue );
	}

I would love to add tests but to be honest I'm drawing a blank on how to test these changes. Advice is welcome.

@seancorfield

This comment has been minimized.

Show comment
Hide comment
@seancorfield

seancorfield Feb 21, 2017

Member

The mechanics look good (although this introduces some overhead, especially around flash scope, but that's unavoidable for pluggability).

My only comment, from a stylistic p.o.v., is that arguments. is not "house style" for FW/1 -- arguments should be referred to unqualified.

You're right that tests will be hard to write for this.

Member

seancorfield commented Feb 21, 2017

The mechanics look good (although this introduces some overhead, especially around flash scope, but that's unavoidable for pluggability).

My only comment, from a stylistic p.o.v., is that arguments. is not "house style" for FW/1 -- arguments should be referred to unqualified.

You're right that tests will be hard to write for this.

@atuttle

This comment has been minimized.

Show comment
Hide comment
@atuttle

atuttle Feb 22, 2017

Contributor

No problem, I'll pull the arguments. prefixes. 👍

Contributor

atuttle commented Feb 22, 2017

No problem, I'll pull the arguments. prefixes. 👍

@seancorfield seancorfield merged commit 1146331 into framework-one:develop Feb 22, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@seancorfield

This comment has been minimized.

Show comment
Hide comment
Member

seancorfield commented Feb 22, 2017

👍

@atuttle atuttle deleted the AlumnIQ:overridable-session-access branch Feb 22, 2017

@seancorfield seancorfield modified the milestone: 4.1 Feb 22, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment