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

Refactoring #22

Merged
merged 5 commits into from
Dec 17, 2017
Merged

Refactoring #22

merged 5 commits into from
Dec 17, 2017

Conversation

Bukashk0zzz
Copy link
Collaborator

Hi,

Pull request contains:

  • Codestyle rules
  • First test and settings
  • Fixed dependencies
  • Small bug fixes
  • Update license now it correct in composer and GitHub
  • Add support Symfony 4 and Symphony Flex
  • Dropped support PHP < 7.1 and Symfony 3.3

If you agree with this changes. It will be good if you give me write access to this repo. I will add more linters for this project + will add some checks in configuration.

If you think so big changes not good idea thats ok. I will do all the things in my fork.

@thecatontheflat
Copy link
Owner

Hey! This looks awesome, thank you so much for taking your time and creating the PR!

The only concern I have is escaping the native functions such as json_encode(), explode(), count() etc. I think it contributes to the worse readability.

Is specifying global namespace for those functions really necessary?

@Bukashk0zzz
Copy link
Collaborator Author

Bukashk0zzz commented Dec 17, 2017

@thecatontheflat Starting from 7.1 this is important for opcache optimization

@thecatontheflat
Copy link
Owner

Thanks a lot for this!

@thecatontheflat thecatontheflat merged commit e0863a6 into thecatontheflat:master Dec 17, 2017
@thecatontheflat
Copy link
Owner

Created 2.0.0 tag

@Bukashk0zzz
Copy link
Collaborator Author

@thecatontheflat Can you please add

Your repo not in organization so I can't do this.
This not take much time but repo will looks much better.
When you registered this repo in this tools I will add badges and update readme.
Thanks.

@Bukashk0zzz
Copy link
Collaborator Author

Also thanks for merging.

@thecatontheflat
Copy link
Owner

@Bukashk0zzz Could you do this if I added you to the co-owners of this repo?

@Bukashk0zzz
Copy link
Collaborator Author

https://travis-ci.org -- not sure but others think yes.

@thecatontheflat
Copy link
Owner

Activated this project on Travis, sent you a Collaborator's invite

Cheers!

@Bukashk0zzz
Copy link
Collaborator Author

@thecatontheflat Thank you.

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