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

Session: added getFlashSection() #5

Closed
wants to merge 1 commit into from
Closed

Session: added getFlashSection() #5

wants to merge 1 commit into from

Conversation

dg
Copy link
Member

@dg dg commented Jul 1, 2014

No description provided.

public function getFlashSection($section)
{
if (!$this->flashId) {
$this->flashId = Nette\Utils\Random::generate(4) ?: '1';
Copy link
Contributor

Choose a reason for hiding this comment

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

Random::generate() can fail?

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

@Majkl578
Copy link
Contributor

Majkl578 commented Jul 3, 2014

Not sure why, but I don't like this. It's completely out of Session's scope. Also this is only a nette\application thing. Not sure it should be hardcoded into Session this way.

@mishak87
Copy link

mishak87 commented Jul 3, 2014

This whole code should be object dependent on Session new FlashSessionStorage($this->getSession()) and in nette/application.

@dg
Copy link
Member Author

dg commented Jul 3, 2014

I have two implementations. This is one and the second one uses new class Nette\Http\FlashSession. I'm not sure which way to go. But it is definitely not only nette/application thing, because it solves common problem with session ID identifier.

@mishak87
Copy link

mishak87 commented Jul 3, 2014

@dg Sorry I had in mind Tab and after searching just found out you changed the name back from Tab to Flash. nette/application#4 (comment)

Where is the second one?

@dg
Copy link
Member Author

dg commented Jul 3, 2014

The second one is only in my local branch.

„Tab“ idea was completely wrong. I realized that the important is that it exists only during the redirect.

@mishak87
Copy link

mishak87 commented Jul 3, 2014

Instead of explaining further I have created two pulls it should be clear from them what I meant.

NOTE: Separating the session namespace from id is great idea the class just extends on that.

@dg dg force-pushed the master branch 4 times, most recently from 287cae9 to f55bed2 Compare September 5, 2014 21:34
@dg dg force-pushed the master branch 9 times, most recently from 0bb4336 to 96b498c Compare December 27, 2014 07:22
@dg dg closed this Jan 25, 2015
@dg dg deleted the pull-tab branch January 25, 2015 19:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants