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

added getSegment method to Segment class to return the entire segment at once #41

Closed
wants to merge 1 commit into from

Conversation

epoplive
Copy link

No description provided.

@harikt
Copy link
Member

harikt commented Apr 17, 2015

are you trying to get all the values from the Segment?

And I am not in favour of having the name getSegment inside the Segment itself. May be getValues or even we can alter the get method.

ie when no key is there return the session

public function get($key, $alt = null)
{
$this->resumeSession();
return isset($_SESSION[$this->name][$key])
? $_SESSION[$this->name][$key]
: $alt;
}

Also this is another BC break.

@epoplive
Copy link
Author

Yes, I need to get all of the values for the segment. I'm not really big on the name either, I thought of getValues but didn't really feel like that conveys the purpose, and getAllValues() or getEntireSegment() didn't seem particularly great. Open to suggestions, just need this functionality.

I spoke with pmjones earlier on IRC, and he prefers to not modify the existing methods, and even so modifying the existing method to take a null for the first param becomes ugly if one chooses a default besides null.

And again, adding new functionality is not breaking backwards compatibility. This is not legacy software...perhaps you thought you were looking at the 1.x branch?

@epoplive
Copy link
Author

talked with pmjones, he's going to fork a 3.x branch so I can start making pull requests against that. Problem solved for everyone it sounds like.

@epoplive epoplive closed this Apr 17, 2015
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.

2 participants