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

Ready to use in Symfony3 #28

Merged
merged 6 commits into from
Apr 12, 2017
Merged

Ready to use in Symfony3 #28

merged 6 commits into from
Apr 12, 2017

Conversation

terox
Copy link
Contributor

@terox terox commented Mar 20, 2017

Hello!

I have changed some files to do this bundle compatible with Symfony2/3.

@kieljohn
Copy link
Member

Hi Terox!

Thanks for your contribution, our team will take a look at this and try to get it merged

@@ -37,7 +28,7 @@ public function setLength($length)
*/
public function generate()
{
return $this->secureRandom->nextBytes($this->length);
return random_bytes($this->length);
Copy link
Contributor

@leightonthomas leightonthomas Mar 28, 2017

Choose a reason for hiding this comment

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

random_bytes() only seems to be available in PHP 7 - perhaps https://github.com/symfony/polyfill-php70 could be used?

edit: didn't realise this was already included by symfony security stuff, my bad!

Copy link
Contributor

Choose a reason for hiding this comment

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

However, I think if we're going to rely on random_bytes, we need to be explicit in the polyfill package as a dependency of ours.

We can't necessarily rely on the security component including it.

@leightonthomas
Copy link
Contributor

leightonthomas commented Mar 28, 2017

.travis.yml will need updating due to the PHP version requirement change and I think there's some phpspec test issues that need resolving as well, other than that looks good

Copy link
Contributor

@Brunty Brunty left a comment

Choose a reason for hiding this comment

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

Also, tests will need fixing before this is merged.

composer.json Outdated
"doctrine/common": "~2.2",
"doctrine/orm": "~2.2",
"symfony/security": "~2.1",
"symfony/options-resolver": "~2.1",
"symfony/security": "~2.8|~3.2",
Copy link
Contributor

Choose a reason for hiding this comment

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

If this is to support Symfony 3 as well, then what about 3.0 and 3.1?

I'd suggest a change to: ^2.8|^3.0

composer.json Outdated
"symfony/security": "~2.1",
"symfony/options-resolver": "~2.1",
"symfony/security": "~2.8|~3.2",
"symfony/options-resolver": "~2.8|~3.2",
Copy link
Contributor

Choose a reason for hiding this comment

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

Same requirements change on this as the symfony/security

@@ -37,7 +28,7 @@ public function setLength($length)
*/
public function generate()
{
return $this->secureRandom->nextBytes($this->length);
return random_bytes($this->length);
Copy link
Contributor

Choose a reason for hiding this comment

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

However, I think if we're going to rely on random_bytes, we need to be explicit in the polyfill package as a dependency of ours.

We can't necessarily rely on the security component including it.

@Brunty
Copy link
Contributor

Brunty commented Mar 31, 2017

Nice work, thanks @terox!

@terox
Copy link
Contributor Author

terox commented Mar 31, 2017

Please, could you check the changes? I removed the PHP 5.4 from .travis.yml because the mininum requeriments for Symfony 2.8 and Symfony 3 I think that is 5.5

@kieljohn
Copy link
Member

Looks good, thanks for the contribution. We'll merge and tag a version early next week 👍

@terox
Copy link
Contributor Author

terox commented Mar 31, 2017

Thank you so much for this fantastic bundle ;)

@kieljohn kieljohn merged commit 501f181 into vivait:master Apr 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants