search

mothership-ec / cog

Cog
Other
2 stars 3 forks source link

Some RNG enhancements #417

Closed sarciszewski closed 9 years ago

sarciszewski commented 9 years ago
kuiche commented 9 years ago

Hi,

Thanks for this. There is one problem I can see - validation with that pattern will result in an infinite loop if the length param is set in the function call. e.g. $generator->generate(10) will result in an infinite loop. The pattern should be made to match the length passed in.

If we are validating the length like this we should also ensure an infinite loop can't happen - we should have a max number of iterations before failure so we actually get an error rather than it just appearing to stop.

See https://github.com/mothership-ec/cog/pull/422 for a basic unit test

sarciszewski commented 9 years ago

You have unit tests but aren't using Travis CI. :(

kuiche commented 9 years ago

Yes, we have unit tests. Unfortunately they're in a bit of a bad place at the moment with quite a few failing simply because they are wrong. We are slowly improving them (we are a small team atm) but we need to get them to a point where they all pass before we consider using CI.

For now they are limited to testing as and when we feel we need them (such as for testing this).

With regards to the pattern validation, this is a good idea and one which looks like there was the intention of implementing. Would it be possible to generate the pattern based on the passed in length and throw an exception if it loops too many times? This shouldn't block this PR going through though. We can add this after.

@thomasjthomasj also pointed out we should probably validate the $length param.

sarciszewski commented 9 years ago

I've made it prevent infinite loops. I don't think the inner loop should ever run more than once. /dev/urandom and openssl are very good about returning the amount of data you request.

kuiche commented 9 years ago

Yeah, they are. It's more because the contents of /dev/urandom and openssl are not specified within mothership so if someone has a buggy/broken version of one of them (very unlikely but possible - maybe I have trust issues) it may end up doing a loop. The exception basically points out that the environment is broken rather than just leaving the thing looping and giving no indication as to why or where it's stuck.

Anyway, this looks fine now :+1: