Skip to content

Conversation

@flip111
Copy link
Contributor

@flip111 flip111 commented Dec 3, 2013

When string length > 1 (5) and only one character ('a') has been given the correct return value should be a string with only that character ('aaaaa'). Ok sure there is nothing random about that, but that's more likely a case to throw a logic exception or something similar.

When string length > 1 (5) and only one character ('a') has been given the correct return value should be a string with only that character ('aaaaa'). Ok sure there is nothing random about that, but that's more likely a case to throw a logic exception or something similar.
@simensen
Copy link
Collaborator

@ircmaxell What do you think of this one? I see in the tests you specifically have array(1, 'a', ''), so I'm guessing there is some security reason why that makes sense over array(1, 'a', 'a'),?

@ircmaxell
Copy link
Owner

It's an invalid input. I don't think that 'aaaaa' would be a correct output. So I'd either keep the same, or throw an exception (InvalidArgumentException). Thoughts?

@simensen
Copy link
Collaborator

I'm of the mindset of some sort of exception might make more sense in both the case of $length == 0 and only one character in the set.

Having the possibility to pass back '' seems dangerous to me. I'm not sure what the point would ever be to allow that? If you ever pass input in that intentionally results in '' I feel like it should be an error condition. I'd vote for an exception.

Personally, if we say that it "makes sense" that you can ask for a zero length random string and to get '' back, it should make sense to ask for a six length random string that only contains the letter a. However, when asking for a six length random string and being returned '' definitely seems broken to me so I think that should be fixed. If you are OK with exceptions, I'd vote for that route.

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.

3 participants