Skip to content
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

Make the session helper injectable #1197

Merged
merged 2 commits into from Sep 14, 2016
Merged

Make the session helper injectable #1197

merged 2 commits into from Sep 14, 2016

Conversation

ronan
Copy link
Contributor

@ronan ronan commented Sep 14, 2016

No description provided.

@pantheon-mentionbot
Copy link

@ronan, thanks for your PR! By analyzing the blame information on this pull request, we identified @greg-1-anderson and @TeslaDethray to be potential reviewers

$session = $this->cache->getData('session');
$this->data = $session;
if (empty($session)) {
$this->data = new \stdClass();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is an anonymous object the best way to represent this? If it needs any methods, could it be a full fledged class like Pantheon\Terminus\Session\Data or would it be well served as an array?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you feel like being lazy with your types, could I recommend class SessionData extends \ArrayObject? You can use it like an array today, but it can also be used for typehinting, and you can add accessors to it later. Win.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dunno. I think this should be an array probably but this thing was a pretty lazy copy from the old version. It'll need to be refactored once Auth() is refactored to use it. For now I say leave this be.

@coveralls
Copy link

coveralls commented Sep 14, 2016

Coverage Status

Coverage remained the same at 14.05% when pulling b5b2ecb on change/di-sessions into ce190dd on master.

@TeslaDethray
Copy link
Contributor

+1

@coveralls
Copy link

coveralls commented Sep 14, 2016

Coverage Status

Coverage remained the same at 14.05% when pulling c03ef0c on change/di-sessions into 3584938 on master.

* Test destroying the session
*/
public function testDestroy() {
$data = [
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extraneous test data.

@coveralls
Copy link

coveralls commented Sep 14, 2016

Coverage Status

Coverage remained the same at 14.05% when pulling 8db4a66 on change/di-sessions into da4f14b on master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 14.05% when pulling 8db4a66 on change/di-sessions into da4f14b on master.

@coveralls
Copy link

coveralls commented Sep 14, 2016

Coverage Status

Coverage remained the same at 14.05% when pulling 8db4a66 on change/di-sessions into da4f14b on master.

@coveralls
Copy link

coveralls commented Sep 14, 2016

Coverage Status

Coverage remained the same at 14.05% when pulling 8db4a66 on change/di-sessions into da4f14b on master.

@ronan ronan merged commit 7aeebf6 into master Sep 14, 2016
@ronan ronan deleted the change/di-sessions branch September 14, 2016 21:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants