Skip to content

Conversation

takeit
Copy link
Contributor

@takeit takeit commented Sep 13, 2015

fixes jackalope/jackalope-doctrine-dbal#298

Even better will be to use rhumsaa/uuid library, imho (so I added it). openssl_random_pseudo_bytes function was failing on PHP 5.3.3.

Thus, this PR also fixes #120

added ramsey/uuid generator
@takeit
Copy link
Contributor Author

takeit commented Sep 14, 2015

@lsmith77 what do you think about this ?

@lsmith77
Copy link
Member

Yeah it might make sense .. then again at this point we might then just consider suggesting people to use this lib directly (ie. also Jackalope Doctrine DBAL) to make it easier to also use something else than uuid4.

/cc @dbu @dantleech

@dbu
Copy link
Member

dbu commented Sep 18, 2015

in phpcr-odm we introduced the possibility to specify a closure to generate the UUID. https://github.com/doctrine/phpcr-odm/blob/master/lib/Doctrine/ODM/PHPCR/Configuration.php#L363-L382

its not elegant that the phpcr-utils classes have static methods - but we could add another static method to set a closure that is set to a static property and then used in generateUuid.
if nothing is explicitly specified, we could check for existence of the ramsey thing, the openssl function and a suitable php version and fallback to the current implementation as a last resort. what do you think of that idea?

on the long run, there should be a PSR for uuid generators that we could inject into jackalope and phpcr-odm. but this will take time, if it ever happens.

@lsmith77
Copy link
Member

lsmith77 commented Oct 4, 2015

yeah I think we should automatically use the library if its available

@dbu
Copy link
Member

dbu commented Oct 5, 2015

@takeit could you update this PR to check for existence of the ramsey uuid library and use that if available, and fallback to our own implementation if not available? but not add ramsey to composer.json.

composer.json Outdated
Copy link
Member

Choose a reason for hiding this comment

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

please move this to a suggest

@takeit
Copy link
Contributor Author

takeit commented Oct 9, 2015

take a look now, please

@dbu dbu merged commit 8a5361e into phpcr:master Oct 10, 2015
@lsmith77 lsmith77 removed the review label Oct 10, 2015
@dbu
Copy link
Member

dbu commented Oct 10, 2015

great, thanks a lot!

@dbu dbu changed the title fixes jackalope/jackalope-doctrine-dbal#298 Use rhumsaa/uuid if available Oct 10, 2015
@benglass
Copy link

We were bit by this bug this morning and it took quite a bit of digging to figure out the fix was to install this other package. I am curious as to why this package is not just required and used by default since the existing uuid generation code seems to have a bug in it under certain circumstances?

@dbu
Copy link
Member

dbu commented Aug 31, 2016

i think the reason was that we support php 5.3 and the uuid lib is 5.4 and newer only.

but at this point we could just bump our own minimal php version, i'd say to 5.5 and do a new minor version with that. and at that point make the uuid lib a hard dependency and throw out our own implementation. do you want to do such a PR?

@benglass
Copy link

@dbu That makes sense as does bumping minimal version since 5.4 is end of life'd

Im on vacation starting tomorrow for a month and traveling so I will not be able to make a PR for this until the end of september. I'll check in when I return to see if you still need it done.

@dbu
Copy link
Member

dbu commented Aug 31, 2016 via email

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.

uuid is the same when creating database for the first time consider using "rhumsaa/uuid" for UUID generation
4 participants